Skip to content

[no-base-to-string] add option to prevent usage of Array .join on non-toString things #3388

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.

Sign up for GitHub

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

Jump to bottom
Closed
lydell opened this issue May 15, 2021 · 5 comments · Fixed by #10287
Closed

[no-base-to-string] add option to prevent usage of Array .join on non-toString things #3388

lydell opened this issue May 15, 2021 · 5 comments · Fixed by #10287
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

@lydell
Copy link
Contributor

lydell commented May 15, 2021

(Note: I couldn’t find any issue template/info about feature requests, I hope this is ok! 🙏 )

Summary: Disallow .join() calls on Arrays that aren’t Array<string>.

Motivation:

I had a function like this:

function noCommonRoot(paths: Array<string>): string {
  return `
I could not find a common ancestor for these paths:

${paths.join("\n")}

Files on different drives is not supported.
  `.trim();
}

I had trouble with using too much string types an mixing stuff up, so I introduced an AbsolutePath type to help me, and replaced string with AbsolutePath in many places.

type AbsolutePath = { tag: "AbsolutePath"; absolutePath: string };

function noCommonRoot(paths: Array<AbsolutePath>): string {
  return `
I could not find a common ancestor for these paths:

${paths.join("\n")}

Files on different drives is not supported.
  `.trim();
}

When I had fixed all compile errors, I had accidentally introduced a bug! noCommonRoot now outputs:

I could not find a common ancestor for these paths:

[object Object]
[object Object]
[object Object]

Files on different drives is not supported.

Oops!

I searched for .join in the entire project and switched to using this function instead, to avoid this problem in the future:

function join(array: Array<string>, separator: string): string {
  return array.join(separator);
}

That way I could fix the noCommonRoot function:

function noCommonRoot(paths: Array<AbsolutePath>): string {
  return `
I could not find a common ancestor for these paths:

${join(
  paths.map((path) => path.absolutePath),
  "\n"
)}

Files on different drives is not supported.
  `.trim();
}

Bonus: my join function requires the separator argument. That would be nice to lint too. The default "," is never what I want.
@lydell lydell added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels May 15, 2021
@bradzacher
Copy link
Member

THis definitely makes sense as something to lint for, but I don't think it should be a brand new rule.

I think this would work well as an extension / additional option in our no-base-to-string rule, which already does these checks, just not on a .join call.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels May 15, 2021
@lydell
Copy link
Contributor Author

lydell commented May 15, 2021

That sounds great! Turns out I’m already using that rule, but didn’t remember it when writing this issue.

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed awaiting response Issues waiting for a reply from the OP or another party labels May 15, 2021
@bradzacher bradzacher changed the title Proposal: No mistakes with Array .join [no-base-to-string] add option to prevent usage of Array .join on non-toString things May 15, 2021
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@FloEdelmann FloEdelmann mentioned this issue Dec 15, 2021
Closed
@safareli

This comment was marked as spam.

Sign in to view
@Josh-Cena Josh-Cena mentioned this issue Aug 23, 2024
Closed
4 tasks
@ronami
Copy link
Member

ronami commented Oct 9, 2024 •
edited
Loading

Would it make sense to also prevent calling .toString() or stringifying a non to-string array (effectively the same as .join())?

// both resolve to `[object Object],[object Object]`
[{}, {}].toString();
`${[{}, {}]}`;

@ronami ronami mentioned this issue Oct 9, 2024
Closed
3 tasks
@bradzacher
Copy link
Member

@ronami The latter case would already be covered by restrict-template-expressions. It's not as granular as "the elements must be toStringable" and would just block the array altogether -- but the majority of the time that would be correct.

@yeonjuan yeonjuan mentioned this issue Nov 5, 2024
Merged
3 tasks
@kirkwaiblinger kirkwaiblinger mentioned this issue Dec 1, 2024
Closed
4 tasks
@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 Dec 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Milestone
No milestone
5 participants
@safareli @lydell @JoshuaKGoldberg @ronami @bradzacher