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

feat: allow to set web/webworker and node environments #18336

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexander-akait
Copy link
Member

What kind of change does this PR introduce?

Feature. Why we need to expose them:

Why we can't use just target? Because developer can have target: "browserslist" and to undestan the right enviroment we need to do load and resolve it (webpack already does it, so no need to duplicate it)

Also there is a fix for bigIntLiteral, because we use the optimistic strategy when nothing avaliable for ECMA features

Did you add tests for your changes?

Yes

Does this PR introduce a breaking change?

NO

What needs to be documented once your changes are merged?

New options

/cc @vankop

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

() => /** @type {boolean | undefined} */ (tp && tp.bigIntLiteral)
() =>
tp &&
optimistic(/** @type {boolean | undefined} */ (tp && tp.bigIntLiteral))
Copy link
Member Author

Choose a reason for hiding this comment

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

We use the same for const/forOf/etc, so let's use the same here when no targets avaliable

@vankop
Copy link
Member

vankop commented Apr 18, 2024

need to think about this carefully.. idea of environment option is to understand does environment support some features ( basically js features now ). Maybe store additional target information? so we wont need to resolve browserlist twice

@vankop
Copy link
Member

vankop commented Apr 18, 2024

something like class Target. Instance of which will be assignment to compiler and then use something like compiler.target.isNode()

@alexander-akait
Copy link
Member Author

@vankop Yeah, I thought about it, but we have target: false (i.e. you can setup your own env), so when you have target: false, you need to ability to setup all envs, so you can flexy setup which features/things your env supports

@vankop
Copy link
Member

vankop commented May 1, 2024

@alexander-akait target: false is a strange edge case.. we can warn on something like this.. but better stick to webpack design and by design target option should be used in this cases.

@vankop
Copy link
Member

vankop commented May 1, 2024

wdyt about this design @alexander-akait #18378 ?

@alexander-akait
Copy link
Member Author

@vankop Yeah, looks good, I put a comment about loader context there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Merge
Development

Successfully merging this pull request may close these issues.

None yet

3 participants