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

[HOLD] fix(worker): Avoid webpack error on "node:" modules in workflows #1037

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mjameswh
Copy link
Contributor

What changed

  • In the workflow bundler, ignored modules get replaced by an empty object from Webpack's externals hook, in addition to aliases.

Why

if (!moduleMatches(module, this.ignoreModules)) {
this.foundProblematicModules.add(module);
}
return 'var {}';
Copy link
Member

Choose a reason for hiding this comment

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

Should we change to return an empty object? Is this better experience in your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean... Do you mean var false vs var {}?

Copy link
Member

Choose a reason for hiding this comment

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

We used to return undefined here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see... Actually, THIS is the fix.

By returning var {}, we are telling Webpack to replace imports of that module by an empty object. This is essentially the same as what we do with alias, but externals is called much earlier in the process, so that we can still handle invalid namespaces such as node:.

- Add comments
- Completely deny usage of the `node:` prefix, even for things modules that would otherwise be allowed
@mjameswh mjameswh marked this pull request as draft January 28, 2023 02:45
@mjameswh
Copy link
Contributor Author

Putting this one on hold.

After some more consideration, I think that the currently proposed solution will make it significantly more difficult for users to load build-time polyfills.

For this reason, it might be preferable that we simply rewire node:* to * from externals, then let alias behave as before, thus making it possible for users to load polyfill by overwriting the corresponding entry in alias.

@mjameswh mjameswh changed the title fix(worker): Avoid webpack error on "node:" modules in workflows [ON HOLD] fix(worker): Avoid webpack error on "node:" modules in workflows Mar 22, 2023
@CLAassistant
Copy link

CLAassistant commented Jul 6, 2023

CLA assistant check
All committers have signed the CLA.

@mjameswh mjameswh changed the title [ON HOLD] fix(worker): Avoid webpack error on "node:" modules in workflows [HOLD] fix(worker): Avoid webpack error on "node:" modules in workflows Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to add module to disallowedModules?
3 participants