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

fix ExportsInfo #15367

Merged
merged 6 commits into from
Feb 14, 2022
Merged

fix ExportsInfo #15367

merged 6 commits into from
Feb 14, 2022

Conversation

vankop
Copy link
Member

@vankop vankop commented Feb 10, 2022

  • should mark every export canMangleProvide=false if there are unknown exports
  • add __webpack_exports_info__.<name>.canMangle to api
  • add test case

What kind of change does this PR introduce?

bugfix
fixes #15214

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?

add __webpack_exports_info__.<name>.canMangle to api

@webpack-bot
Copy link
Contributor

webpack-bot commented Feb 10, 2022

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)

- should mark every export canMangleProvide=false if there are unknown exports
- add __webpack_exports_info__.<name>.canMangle to api
- add test case
@@ -3274,7 +3274,7 @@ cacheable modules 1.22 KiB
ModuleConcatenation bailout: Module is not an ECMAScript module
webpack x.x.x compiled successfully in X ms

asset main.no-side.js 979 bytes [emitted] [minimized] (name: main)
asset main.no-side.js 993 bytes [emitted] [minimized] (name: main)
Copy link
Member Author

@vankop vankop Feb 10, 2022

Choose a reason for hiding this comment

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

no mangling for one variable (same case with empty js file)

const exportsInfo = moduleGraph.getExportsInfo(module);
const exportInfo = exportsInfo.getExportInfo(exportName);
if (exportInfo) return exportInfo.canMangle;
return exportsInfo.otherExportsInfo.canMangleProvide;
Copy link
Member

Choose a reason for hiding this comment

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

Why is that canMangleProvide and not canMangle?

Suggested change
return exportsInfo.otherExportsInfo.canMangleProvide;
return exportsInfo.otherExportsInfo.canMangle;

@@ -0,0 +1,2 @@
/** @type {import("../../../../").Configuration} */
module.exports = {};
Copy link
Member

Choose a reason for hiding this comment

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

When this doesn't need special config, we should move it into test/cases so it's tested with different configs.

Maybe wrapping some assertions with if(process.env.NODE_ENV === "production") (e. g. for optimizations that only run in production mode)

Copy link
Member Author

Choose a reason for hiding this comment

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

this was done intentionally to run 3 times (check for cache invalidation)

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes that makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

moved to test/cases added cache invalidation stuff there as well

@webpack-bot
Copy link
Contributor

@vankop Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

test: "no-string"
},
{
// Pack got invalid because of write to: ResolverCachePlugin|normal|dependencyType=|esm|path=|/Users/ivankopeykin/Repositories/webpack/test/cases|request=|./large/big-assets/
Copy link
Member Author

Choose a reason for hiding this comment

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

fix in #15373

infrastructureLogErrors: {
compile: {
// Module build failed
["error-hide-stack"]:
Copy link
Member Author

@vankop vankop Feb 14, 2022

Choose a reason for hiding this comment

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

maybe add this to errors.js file? cache-errors.js maybe?

filter: [logErrors.PERSISTENCE_CACHE_INVALIDATE_ERROR]
wasm: {
// Can not compile wasm module
["missing-wasm-experiment"]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
["missing-wasm-experiment"]:
"missing-wasm-experiment":

That's not necessary... but you can leave that for now

@sokra
Copy link
Member

sokra commented Feb 14, 2022

Thanks

@webpack-bot
Copy link
Contributor

I've created an issue to document this in webpack/webpack.js.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect cache invalidation using module with side effects and re-exporting from empty file
3 participants