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

prefer-destructuring conflicts with Webpack replacements of process.env #14918

Closed
fregante opened this issue Aug 11, 2021 · 5 comments
Closed
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon
Projects

Comments

@fregante
Copy link
Contributor

fregante commented Aug 11, 2021

Webpack can replace Node-like process.env.ENV_STUFF with the actual environment variable provided at build time: https://webpack.js.org/plugins/environment-plugin/

This process is pretty "dumb" as it's a simple search and replace of exactly process.env.${variableName} and nothing else. This means destructuring isn't possible with process.env.

More context found in: vercel/next.js#19420

What rule do you want to change?

prefer-destructuring

Does this change cause the rule to produce more or fewer warnings?

Fewer warnings

How will the change be implemented? (New option, new default behavior, etc.)?

New default I'd assume

Please provide some example code that this change will affect:

const ENVIRONMENT = process.env.ENVIRONMENT

What does the rule currently do for this code?

It suggests fixing it to this, breaking webpack’s replacement.

const {ENVIRONMENT} = process.env;

What will the rule do after it's changed?

Ignore process.env destructuring. It'd be great to even suggest the opposite (Avoid destructuring of process.env), but maybe that's asking too much.

@fregante fregante added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Aug 11, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Aug 11, 2021
@nzakas
Copy link
Member

nzakas commented Aug 12, 2021

We generally try to avoid ultra-specific exceptions like this. This seems like a good case for using a disable comment instead.

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Aug 12, 2021
@nzakas
Copy link
Member

nzakas commented Aug 18, 2021

I took another look at this, and it seems like it would be pretty trivial for webpack to replace destructured environment variables with individual variable declarations. I think approaching this issue from the webpack side is probably a better approach.

@nzakas nzakas closed this as completed Aug 18, 2021
Triage automation moved this from Feedback Needed to Complete Aug 18, 2021
@fregante
Copy link
Contributor Author

I found the issue on Webpack’s repo, currently open for 4 years 😰 probably not pretty trivial

@fregante
Copy link
Contributor Author

Would it be possible to add a generic "exclude" option? So the user can specify which patterns to exclude based on the variable name

@njbmartin
Copy link

Just ran into this issue myself, I definitely prefer destructuring objects and don't particularly want to add comments to disable (as it's in a number of files.

As an example, having to do this:

/*eslint-disable */
const KLAVIYO_LIST_ID = process.env.KLAVIYO_LIST_ID;
const KLAVIYO_API_KEY = process.env.KLAVIYO_API_KEY;
const FAUNADB_SECRET = process.env.FAUNADB_SECRET;
const FAUNADB_DOMAIN = process.env.FAUNADB_DOMAIN;
/* eslint-enable */

It would be great if there was a way to add something in the eslint config to ignore process.env for this rule.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 15, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

3 participants