Skip to content

Fix support for Webpack externals. This fixes #471 #495

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

Merged
merged 8 commits into from
Feb 2, 2019
Merged

Fix support for Webpack externals. This fixes #471 #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
84a0dc0
Store externals in an array. This fixes #471
Dec 13, 2018
3d6fb55
Ensure a copy of the externals is returned when generating a Webpack …
Lyrkan Dec 14, 2018
a51b35f
Update documentation for Encore.addExternals
Dec 17, 2018
5ab6822
Include a regex in the tests of addExternals.
Jan 7, 2019
0e012d3
Fix style issues identified by lint.
Jan 21, 2019
c0a8d8d
Use {*} instead of {object} for externals parameter of addExternals m…
Jan 22, 2019
c1fbbf3
Split the addExternals example into two separate examples.
Jan 22, 2019
58dc86d
tweaking example
weaverryan Feb 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,9 +390,19 @@ class Encore {
* Encore.addExternals({
* jquery: 'jQuery',
* react: 'react'
* })
* });
*
* Or:
*
* const nodeExternals = require('webpack-node-externals');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if that's a good idea to put this specific example. I'm a bit scared that some people will think this is a requirement in order to add externals...

Copy link
Author

@deAtog deAtog Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Feel free to suggest an alternate example, but webpack-node-externals is probably the most commonly used functional usage here. Maybe spitting it into two examples would be better? One that only adds jquery and react, and one that uses webpack-node-externals?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... maybe splitting it would be better yes.
But maybe I'm also worrying for no real reason here...

@weaverryan What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I think splitting is better since I really can't see someone defining 'jquery' and 'react' as externals and then using webpack-node-externals in the same config.

*
* Encore.addExternals(
* // add any valid externals you have
* nodeExternals(),
* /^(jquery|\$)$/i
Copy link
Collaborator

Choose a reason for hiding this comment

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

That PR is the one that doesn't use rest parameters (as opposed to #472), so you'd need to put these two lines into an array.

* );
*
* @param {object} externals
* @param {*} externals
*
* @returns {Encore}
*/
Expand Down
10 changes: 5 additions & 5 deletions lib/WebpackConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class WebpackConfig {
this.providedVariables = {};
this.configuredFilenames = {};
this.aliases = {};
this.externals = {};
this.externals = [];

// Features/Loaders flags
this.useVersioning = false;
Expand Down Expand Up @@ -298,12 +298,12 @@ class WebpackConfig {
Object.assign(this.aliases, aliases);
}

addExternals(externals = {}) {
if (typeof externals !== 'object') {
throw new Error('Argument 1 to addExternals() must be an object.');
addExternals(externals = []) {
if (!Array.isArray(externals)) {
externals = [externals];
}

Object.assign(this.externals, externals);
this.externals = this.externals.concat(externals);
}

enableVersioning(enabled = true) {
Expand Down
2 changes: 1 addition & 1 deletion lib/config-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class ConfigGenerator {
config.resolve.alias['react-dom'] = 'preact-compat';
}

config.externals = Object.assign({}, this.webpackConfig.externals);
config.externals = [...this.webpackConfig.externals];

return config;
}
Expand Down
21 changes: 7 additions & 14 deletions test/WebpackConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,24 +940,17 @@ describe('WebpackConfig object', () => {
it('Adds new externals', () => {
const config = createConfig();

expect(config.externals).to.deep.equals({});
expect(config.externals).to.deep.equals([]);

config.addExternals({ 'jquery': 'jQuery', 'react': 'react' });
config.addExternals({ 'lodash': 'lodash' });
config.addExternals(/^(jquery|\$)$/i);

expect(config.externals).to.deep.equals({
'jquery': 'jQuery',
'react': 'react',
'lodash': 'lodash'
});
});

it('Calling it with an invalid argument', () => {
const config = createConfig();

expect(() => {
config.addExternals('foo');
}).to.throw('must be an object');
expect(config.externals).to.deep.equals([
{ 'jquery': 'jQuery', 'react': 'react' },
{ 'lodash': 'lodash' },
/^(jquery|\$)$/i
]);
});
});

Expand Down
6 changes: 3 additions & 3 deletions test/config-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ describe('The config-generator function', () => {

const actualConfig = configGenerator(config);

expect(actualConfig.externals).to.deep.equals({});
expect(actualConfig.externals).to.deep.equals([]);
});

it('with addExternals()', () => {
Expand All @@ -485,10 +485,10 @@ describe('The config-generator function', () => {

const actualConfig = configGenerator(config);

expect(actualConfig.externals).to.deep.equals({
expect(actualConfig.externals).to.deep.equals([{
'jquery': 'jQuery',
'react': 'react'
});
}]);
});
});

Expand Down