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

Change Request: Add option to allow NODE_ENV in n/no-process-env #283

Closed
1 task
alecmev opened this issue May 24, 2024 · 4 comments · Fixed by #345
Closed
1 task

Change Request: Add option to allow NODE_ENV in n/no-process-env #283

alecmev opened this issue May 24, 2024 · 4 comments · Fixed by #345

Comments

@alecmev
Copy link

alecmev commented May 24, 2024

eslint-plugin-n version

17.7.0

What problem do you want to solve?

It's common for bundlers to inline process.env.NODE_ENV and such, so that minifiers can do their job and eliminate dead code. Currently n/no-process-env takes issue with every use of process.env.NODE_ENV and I have to disable it manually. I don't want to disable it completely as I still want to encourage configuration validation. I can't import nodeEnv from ./config.js or something because some (all?) minifiers don't dig that far and will not eliminate any code wrapped in if (nodeEnv === "production") ....

What do you think is the correct solution?

Options:

  1. Add allowNodeEnv, defaulting to false. Probably a bit simpler to implement and covers the majority of cases.
  2. Add allowlist, defaulting to []. NODE_ENV is the behemoth, but there's a long tail of other variables.

Participation

  • I am willing to submit a pull request for this change.

Additional comments

No response

@scagood
Copy link

scagood commented May 24, 2024

It sounds to me like you may be better off disabling the rule in general if you have a long list of variables you want to use 🤔

@alecmev
Copy link
Author

alecmev commented May 24, 2024

I personally don't have a long list, just NODE_ENV 😄 So would be happy with allowNodeEnv. I mention other variables because who knows what others might want to allow.

@scagood
Copy link

scagood commented May 24, 2024

Ah, I see!

Looking at the source, it looks like we only look for the process.env (https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/rules/no-process-env.js#L11-L16), I wonder if we should add a check to look for what property on process.env is called, or only report the use of specific env vars 🤔

@voxpelli
Copy link
Member

Add allowlist, defaulting to []

This is what I would add. Good suggestion!

@scagood scagood added rule:update An update to a current rule and removed rule labels Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants