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

Support destructuring #14800

Open
3 of 4 tasks
vankop opened this issue Nov 23, 2021 · 12 comments
Open
3 of 4 tasks

Support destructuring #14800

vankop opened this issue Nov 23, 2021 · 12 comments

Comments

@vankop
Copy link
Member

vankop commented Nov 23, 2021

Feature request

support destructuring in:

What is the expected behavior?

Webpack should support destructuring on parser level

What is motivation or use case for adding/changing the behavior?

Modern versions of runtime environments (browsers, Node.js, Deno) support destructuring

How should this be implemented in your opinion?

JavascriptParser should be improved

Are you willing to work on this yourself?
yes

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@fregante
Copy link

fregante commented Mar 3, 2022

6 other issues were redirected to this one, let's not make it die

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

@TheLarkInn
Copy link
Member

EnvironmentPlugin, DefinePlugin, ImportMetaPlugin (partial support?), other plugins?

@vankop this piece is already supported now correct?

@vankop
Copy link
Member Author

vankop commented Mar 10, 2023

@TheLarkInn webpack doesnt support destructing.. so what actually happens right now:

const { a } = process.env;

webpack renders to:

const { a } = {...all envs here...};

issue is about providing only required properties:

const { a } = { a: 1 };

main reason why it didn't solved yet, as we discussed with @sokra we need to support <es2015 ( mainly babel version ) as well to make build result independent to target. otherwise we could have:

const { a } = { a: 1 };

for >=es2015 and

var _b = {...all envs...},
  a = _b.a;

for es<2015

but supporting babel version is tricky.. I would vote to drop this and support only destructing

@fregante
Copy link

fregante commented Mar 10, 2023

Last time I tried, process.env; was not replaced at all because EnvironmentPlugin and DefinePlugin did a string search and replace for exactly "process.env.NAME". Has this changed since? What you're saying only worked if you used something along the lines of DefinePlugin({'process.env':{NAME: 'foo'}})

@vankop
Copy link
Member Author

vankop commented Mar 10, 2023

What you're saying only worked if you used something along the lines of DefinePlugin({'process.env':{NAME: 'foo'}})

that is a correct usage.. not sure what you mean..


if you define process.env.a and use const { a } = process.env; it will not work

@fregante
Copy link

that is a correct usage.. not sure what you mean..

"Correct" but far from ideal since EnvironmentPlugin exists for this reason but it exclusively supports process.env.a

@vankop
Copy link
Member Author

vankop commented Mar 10, 2023

yes, api is not optimal, but breaking change could lead to security leaks like:

webpack.config.js

const define = new DefinePlugin({'env': {..blabla}, 'env.SECRET': 1});
module.exports = [{
  name: 'my-awesome-backend',
  entry: 'backend.js',
  plugins: [define]
}, {
 name: 'my-awesome-frontend',
  entry: 'frontend.js',
  plugins: [define]
}];

backend.js

// I love to use secrets on backend since it is quite save
const { SECRET } = env;
// or case that works right now
env.SECRET;

frontend.js

// I dont like secrets in my frontend to be exposed
const { data } = env;

Thats why we cant fix api right now

@fregante
Copy link

fregante commented Mar 10, 2023

For the record, I don't care about DefinePlugin specifically, I only mentioned it because "it's the only way to do it". EnviromentPlugin is the only plugin I would use to set ENVs.

The argument in #5392 is that I would expect:

new EnvironmentPlugin(['NAME'])

to support both const NAME = process.env.NAME and const {NAME} = process.env indiscriminately.

Lint rules like prefer-destructuring effectively push users into webpack’s own limitations and rightfully refuse to "fix" it: eslint/eslint#14918 (comment)

The only security argument I see is against allowing const {[dynamic] = myvar} = process.env, which doesn't apply to EnviromentPlugin anyway


I posted my own repos/tests in:

@KaKi87
Copy link

KaKi87 commented Mar 10, 2023

issue is about providing only required properties:
const { a } = { a: 1 };

This doesn't account for the following though :

const {
  env: {
    a
  }
} = process;

@TheLarkInn
Copy link
Member

Could we try and implement these scenarios behind experiments.supportModernDestructuring or something similar (we can bikeshed the experiment name if needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
webpack 5
Awaiting triage
Status: Priority - High
Development

No branches or pull requests

6 participants