Skip to content
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

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

Merged
merged 2 commits into from Apr 18, 2024

Conversation

Grohden
Copy link
Contributor

@Grohden Grohden commented Apr 17, 2024

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?

@Grohden Grohden requested a review from a team as a code owner April 17, 2024 12:45
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Apr 17, 2024
Copy link

linux-foundation-easycla bot commented Apr 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the documentation Relates to ESLint's documentation label Apr 17, 2024
Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 56acbda
🔍 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
Copy link
Member

This was fixed 2 years ago, sindresorhus/globals#184. Please upgrade your package.

sindresorhus/globals#239 (comment)

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

@aladdin-add
Copy link
Member

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
Copy link
Contributor Author

Grohden commented Apr 17, 2024

@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
Copy link
Member

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.

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
@aladdin-add aladdin-add changed the title chore: include notes about globals in migration-guide docs: include notes about globals in migration-guide Apr 18, 2024
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,thanks! Would like another review before merging.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@nzakas nzakas merged commit fb50077 into eslint:main Apr 18, 2024
19 checks passed
@Grohden Grohden deleted the patch-1 branch April 29, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore This change is not user-facing documentation Relates to ESLint's documentation
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

3 participants