Skip to content

[no-base-to-string] doesn't catch String(…) #4314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
FloEdelmann opened this issue Dec 15, 2021 · 5 comments · Fixed by #10005
Closed

[no-base-to-string] doesn't catch String(…) #4314

FloEdelmann opened this issue Dec 15, 2021 · 5 comments · Fixed by #10005
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@FloEdelmann
Copy link
Contributor

FloEdelmann commented Dec 15, 2021

Repro

const foo = {};

console.log(foo.toString()); // warning (correct)
console.log(String(foo)); // no warning

Expected Result

2 warnings about foo being stringified to [object Object].

Actual Result

Only the first statement triggers a warning, the second does not.

Additional Info

MDN documentation for String(…): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/String

Similar to #2440 and (to a lesser extent) #3388.

@FloEdelmann FloEdelmann added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Dec 15, 2021
@bradzacher
Copy link
Member

Personally I would just ban the usage of String() altogether as there are much better and safer ways to coerce things to strings.

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for team members to take a look labels Dec 15, 2021
@keliel
Copy link

keliel commented Jul 13, 2023

Bumping, because this rule has just become "error" in recommended set.

@Josh-Cena
Copy link
Member

Personally I would just ban the usage of String() altogether as there are much better and safer ways to coerce things to strings.

@bradzacher I'm really curious: what do you recommend as an alternative? MDN (which is... written by me) recommends using String(), because it's the only operation that doesn't fail on symbols, which seems more fool-proof than `${value}`.

@bradzacher
Copy link
Member

@Josh-Cena it depends on the intent of the code as to exactly what you want to use.

For stringification you can break it down into three usecases:

  • user visible strings
  • system visible strings (eg keys)
  • dev visible strings

User visible strings shouldn't ever use String(), period. You should never ever be stringifying an unknown value for the user - some formatting / pretification should occur.

System visible strings are less strict - but it's similarly doubtful you're going to be stringifying an unknown value for your system. If you are generally you want a comprehensive and consistent form like JSON.stringify instead of String() though so it encodes objects/arrays properly.

Dev visible strings are just logs and really "anything goes". It's not common to stringify an unknown value for this case. Again however JSON.stringify is generally better - but if you do truly want to handle every single unknown value - even undefined and Symbol - then a hybrid approach with type guards to leverage String() carefully is better still.

With all that said - realistically the usecases for actually wanting String() are rare because there are better alternatives. But I agree - sometimes you do want it.
If we think there are some legitimate cases - we might encode them in the rule!

@bradzacher
Copy link
Member

@KIMBUM

As per our contributing guide please don't leave bump comments. They don't actually increase priority they just spam everyone subscribed to the thread.

We are a community run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it.

If this issue is important to you — consider being that champion.
If not — please use the subscribe function and wait patiently for someone else to implement this.

@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Nov 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants