Skip to content

fix: fixed work of the non-system type externals for "system" library target #13754

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.

Sign up for GitHub

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

Jump to bottom
Merged
sokra merged 1 commit into from
Jul 12, 2021
Merged

fix: fixed work of the non-system type externals for "system" library target #13754

sokra merged 1 commit into from
Jul 12, 2021

Conversation

@StyleT
Copy link
Contributor

@StyleT StyleT commented Jul 8, 2021

Hi!
This tiny change makes externals with custom type specified work correctly with "system" library target.

Now they only work with "umd" library target because of these lines:

webpack/lib/library/UmdLibraryPlugin.js

Lines 126 to 133 in c181294

const modules = chunkGraph
.getChunkModules(chunk)
.filter(
m =>
m instanceof ExternalModule &&
(m.externalType === "umd" || m.externalType === "umd2")
);
let externals = /** @type {ExternalModule[]} */ (modules);

I see similar issue with AMD bundle type too, but let's start with System.js first ;)

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

yes

Does this PR introduce a breaking change?

I believe not :)

What needs to be documented once your changes are merged?

Nothing :)

@webpack-bot
Copy link
Contributor

webpack-bot commented Jul 8, 2021

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)

@StyleT
Copy link
Contributor Author

StyleT commented Jul 8, 2021

Dear maintainers - it would be great to make & release similar fix for Webpack 4. So if you don't mind, I'll sent PR to v4 branch. This will simplify life for me A LOT.

@StyleT
Copy link
Contributor Author

StyleT commented Jul 9, 2021

@sokra Hi! I see you're trying to fix the CI being unstable sometimes. I believe here tests got failed not because of the changes but rather CI issues. Could you take a look?

@sokra sokra merged commit 6fe0bac into webpack:main Jul 12, 2021
@sokra
Copy link
Member

sokra commented Jul 12, 2021

Thanks

@StyleT StyleT mentioned this pull request Jul 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sokra sokra sokra approved these changes

Assignees

No one assigned

Labels

PR: reviewed-approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@StyleT @webpack-bot @sokra