feat(eslint-plugin): [no-misused-spread] add new rule #8509
Conversation
|
Thanks for the PR, @StyleShit! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 07b69b8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8509 +/- ##
==========================================
- Coverage 88.73% 87.27% -1.47%
==========================================
Files 426 252 -174
Lines 14881 12363 -2518
Branches 4327 3899 -428
==========================================
- Hits 13205 10790 -2415
+ Misses 1534 1303 -231
- Partials 142 270 +128
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Not sure why the tests are failing, seems to pass locally |
| @@ -5,7 +5,7 @@ | |||
| "module": "commonjs", | |||
| "strict": true, | |||
| "esModuleInterop": true, | |||
| "lib": ["es2015", "es2017", "esnext"], | |||
| "lib": ["es2015", "es2017", "esnext", "DOM"], | |||
There was a problem hiding this comment.
this fixes the HTMLElement tests
There was a problem hiding this comment.
This seems like it needs further clarification from #9648
There was a problem hiding this comment.
Looks like this caught an existing violation in our codebase:
/home/runner/work/typescript-eslint/typescript-eslint/packages/website/src/theme/NotFound/Content/index.tsx
16:17 error Using the spread operator on a string can cause unexpected behavior. Prefer `String.split('')` instead @typescript-eslint/no-misused-spread
yarn lint on the repository will need to pass.
(posting this comment here because GitHub won't let me post on an unchanged file)
There was a problem hiding this comment.
haha, seems like we have conflicting rules here 😅
There was a problem hiding this comment.
lol. cc @abrahamguo @kirkwaiblinger from #9834 - looks like this just blatantly disagrees with sindresorhus/eslint-plugin-unicorn#1489 -> https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-spread.md.
Maybe we should just not go for strings at all to start? Seems like maybe it's use-case specific?
There was a problem hiding this comment.
I don't have any strong opinion for either of the sides, so... 🤷♂️
(it's also more of a style thing rather than something that could cause errors, no?)
any objection to removing it @abrahamguo @kirkwaiblinger?
There was a problem hiding this comment.
Here are my suggestions.
- Never ever use
split(""). It doesn't do what you think it does for many scripts. [...str]is fine by principle but may still be surprising and not have the intended behavior, especially for emojis. In fact TC39 now considers strings being iterable as a mistake and all future built-in APIs that accept iterables will reject strings by default. If you really want to spread strings, either disable this rule or useIterator.from, or think about alternatives such asIntl.Segmenter.- An option to allow it is good enough. We can consider changing the default to
true, but my vote is to be strict by default.
There was a problem hiding this comment.
+1 from me to everything Josh has indicated. I think this is a really interesting case of the language going one direction (... iterable strings), lint rules following that direction, then the language realizing the direction being wrong. If other lint rules are asking to use ... on strings then we should file an issue on them saying they should update.
There was a problem hiding this comment.
@JoshuaKGoldberg Sure. However, if you are choosing between str.split('') vs [...str], don't you agree that [...str] is certainly more correct than str.split(''), since str.split('') will break surrogate pairs?
and therefore, the premise of unicorn/prefer-spread is correct, even if [...str] might not be the best choice?
There was a problem hiding this comment.
To be honest, I'm coming back from a couple of weeks of intense conference organizing, and don't have the headspace to re-think this deeply right now. I trust @Josh-Cena's opinion on things. 😄
There was a problem hiding this comment.
Both split("") and ... are only correct under certain assumptions, and practically, most of the time, the former is not much more wrong than the latter. It's fine for unicorn to recommend spreading since it's nominally safer, and if users accept safety at the level of spreading they are free to configure the allow config, but banning it by default exposes such unsafety.
There was a problem hiding this comment.
Seems like my most important comment here is hidden, so I'm assuming you haven't seen it 😅 The tests will still fail 😞 |
…/typescript-eslint into feat/no-misused-spread
|
@JoshuaKGoldberg
Seems like both of them don't follow the references/aliases of Anyway... Solved with a list of builtins (I don't like it, but I have no other idea at this point 😞) |
|
Hi @StyleShit do you think you could find some time recently to finish it? Otherwise we could also take it on since there isn't a lot left |
I'm currently on reserve duty so I don't know whether I'll have time in the near future 😓 Thank you! 🙏 |
|
From #8443 (comment):
|
Closes #748