Skip to content

Store externals in an array. This fixes #471 #472

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

Closed
wants to merge 6 commits into from
Closed

Store externals in an array. This fixes #471 #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 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
13b1780
Use ES6 rest parameters for the addExternals method.
Dec 17, 2018
fc1e1f0
Update documentation for Encore.addExternals
Dec 17, 2018
76e6a95
Use ES6 syntax for adding externals to be consistent with the new API
Jan 2, 2019
4fc6dc3
Include a regex in the tests of addExternals.
Jan 7, 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
19 changes: 12 additions & 7 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,17 +387,22 @@ class Encore {
*
* For example:
*
* Encore.addExternals({
* jquery: 'jQuery',
* react: 'react'
* })
* const nodeExternals = require('webpack-node-externals');
*
* Encore.addExternals(
* {
* jquery: 'jQuery',
* react: 'react'
* },
* nodeExternals()
* );
*
* @param {object} externals
* @param {...*} externals
*
* @returns {Encore}
*/
addExternals(externals) {
webpackConfig.addExternals(externals);
addExternals(...externals) {
webpackConfig.addExternals(...externals);

return this;
}
Expand Down
10 changes: 3 additions & 7 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,8 @@ class WebpackConfig {
Object.assign(this.aliases, aliases);
}

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

Object.assign(this.externals, externals);
addExternals(...externals) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking but I'm not sure that rest parameters are needed there since this is not part of the exposed API.
It adds a bit of unneeded complexity to the call in index.js (you have to use apply instead of just doing webpackConfig.addExternals(externals)).

Copy link
Author

Choose a reason for hiding this comment

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

apply is not required, it could have been written as webpackConfig.addExternals(...externals) using ES6 syntax, but I figured webpackConfig.addExternals.apply(webpackConfig, externals) might be more performant as it simply passes the array as the arguments list instead of possibly building a new one. I debated whether or not to use ES6 rest parameters here as I could have simply used the arguments array instead, but I was trying to be a little more explicit in the API declaration. From what I can tell, Babble will convert the ES6 syntax to apply regardless so they should be equally performant. I will change it to the ES6 syntax if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean Babel? If that's the case it's not relevant here since our code don't go through Babel.

Having ...externalsmay be a bit easier to understand (and Node probably optimize it... and even if that's not the case it won't be a big overhead anyway :)) but I still don't really see the benefits of using rest parameters here instead of simply passing the array directly.

Copy link
Author

Choose a reason for hiding this comment

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

The reason for using rest parameters was to simplify the API and the function itself. By using rest parameters or the arguments array, there's no need to check if the parameter is an array in order to perform the concatenation. Users can still pass an array if they want, it will just add the array to the externals list instead of appending each element. Both mechanisms work.

I meant Babel, but seeing as how it is not used, it's irrelevant. Nonetheless I would hope Node internally just converts the ES6 syntax to an apply call. I will change the code to the ES6 syntax to be consistent with the new API.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I agree with @Lyrkan. Look at the "mixed" syntax: https://webpack.js.org/configuration/externals/#combining-syntaxes

Basically, when you call addExternal(), there should only be 3 valid things that you can pass to it:

Encore
     // an object, though the value of each entry can be a string, array or object - but we don't care
     .addExternal({jquery: 'jQuery'})

    // a function
    .addExternal(function(context, request, callback) { /*... */ })

    // regex
    .addExternal(/^(jquery|\$)$/i)

The current implementation allows you to pass these are multiple arguments for multiple externals. That's nice - but I'd rather just accept 1 argument and check that it's one of these 3 types.

@deAtog Could you make that change? If not - just let us know. Sorry for the delay :)

Copy link
Author

@deAtog deAtog Jan 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Consider the following:

Encore
   .addExternals({jquery: 'jQuery'})
   .addExternals(function(context, request, callback) { /*...*/ })
   .addExternals(/^(jquery|\$)$/i)

With my initial patch set this would be functionally equivalent to:

Encore
   .addExternals([
      {jquery:'jQuery'},
      function(context, request, callback) { /*...*/ },
      /^(jquery|\$)$/i
   ])

With the change to rest parameters it is functionally equivalent to:

Encore
   .addExternals(
      {jquery:'jQuery'},
      function(context, request, callback) { /*...*/ },
      /^(jquery|\$)$/i
   )

Switching to rest parameters does not invalidate the second example above, it just produces a slightly different, but still valid configuration. Given the name of the function, I tend to prefer the last version as users don't have to remember to pass an array to addExternals function.

I purposely removed the type checking as just about anything can be provided as an external and it would be better to let Webpack report an issue with an external rather than Encore enforce rules which may change in the future. That is after all where this issue originally started.

Also, there are 5 things an external can be:

  1. a string
  2. an array (including an array of arrays)
  3. an object
  4. a function
  5. a regex

When it is not one of these 5, Webpack currently ignores it, so I don't know why Encore should produce an error in that case.

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' });
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this test by adding, for example, another addExternal() that passes a regex?

Copy link
Author

Choose a reason for hiding this comment

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

I'll update this to include a test for a regex.

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