eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
24.39k stars 4.4k forks source link

docs: include notes about globals in migration-guide #18356

Closed Grohden closed 1 month ago

Grohden commented 1 month ago

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

I'm trying to not get too much frustrated with the migration & migration guides.. so I thought I could contribute on the issues I have faced, hopefully they're welcome 🙏

[x] Documentation update [ ] Bug fix (template) [ ] New rule (template) [ ] Changes an existing rule (template) [ ] Add autofix to a rule [ ] Add a CLI option [ ] Add something to the core [ ] Other, please explain:

What changes did you make? (Give an overview)

Added a note about the required instalation of the globals package (docs don't make it clear that globals is not a magic package and that we need to make it a direct dev dependency)

Is there anything you'd like reviewers to focus on?

linux-foundation-easycla[bot] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

netlify[bot] commented 1 month ago

Deploy Preview for docs-eslint ready!

Name Link
Latest commit 56acbda289e1869b5645bb5bf112c20a71e0c9cc
Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/66211a060c7b940008cfe83e
Deploy Preview https://deploy-preview-18356--docs-eslint.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

aladdin-add commented 1 month ago

This was fixed 2 years ago, https://github.com/sindresorhus/globals/pull/184. Please upgrade your package.

https://github.com/sindresorhus/globals/issues/239#issuecomment-1968731224

When installing it, you will always get the latest version. This change may be more distracting to users.

aladdin-add commented 1 month ago

I would suggest a more appropriate place to be: https://eslint.org/docs/latest/use/troubleshooting/. Let's hear from the others. @eslint/eslint-team

Grohden commented 1 month ago

@aladdin-add

When installing it, you will always get the latest version. This change may be more distracting to users.

You have to remember that some devs (like me) are stupid 😅, I thought this was some magic global package from node or something ("it worked before, I should just change the code to what the guide tells me right?"), so I just imported the package and saw that it was already installed in node_modules, so I thought that eslint included it as a (optional?) dependency on its own to facilitate things, then I got that error.

About the troubleshooting: I'm already trying to find a way to solve the global problem.. why do I have to hunt in another potentially unrelated place the solution? For the specific string search on docs (whitespace issue) I guess it makes sense to be there, but saying "hey, this is not magic, we included the package for you before, but now you'll need to install and use it yourself" would be helpful

aladdin-add commented 1 month ago

About the troubleshooting: I'm already trying to find a way to solve the global problem.. why do I have to hunt in another potentially unrelated place the solution? For the specific string search on docs (whitespace issue) I guess it makes sense to be there, but saying "hey, this is not magic, we included the package for you before, but now you'll need to install and use it yourself" would be helpful

you should always install the package - even if you have not encountered this problem. It may be working without explicitly installing as it may be an indirect dependency, while your package manager (like npm v3+) just flattened it. But it should be avoided.