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

deAtog · 2018-12-13T19:28:33Z

No description provided.

Lyrkan

Thanks for working on that issue!

In addition to the few comments could you also update the JSDoc of the addExternals method in the index.js file? (it helps IDEs' autocompletion)

webpack-encore/index.js

Lines 383 to 403 in b5750b2

    
               /** 
        
                * Allow you to exclude some dependencies from the output bundles. 
        
                * 
        
                * See https://webpack.js.org/configuration/externals/ 
        
                * 
        
                * For example: 
        
                * 
        
                *      Encore.addExternals({ 
        
                *          jquery: 'jQuery', 
        
                *          react: 'react' 
        
                *      }) 
        
                * 
        
                * @param {object} externals 
        
                * 
        
                * @returns {Encore} 
        
                */ 
        
               addExternals(externals) { 
        
                   webpackConfig.addExternals(externals); 
        
                   return this; 
        
               }

lib/config-generator.js

test/WebpackConfig.js

…config. Co-Authored-By: deAtog <[email protected]>

deAtog · 2018-12-17T23:02:40Z

I modified the addExternals interface slightly to allow multiple externals to be added without having to explicitly provide an array and updated the documentation accordingly. Overall, I feel as though this is a slightly better approach as it reduces the burden on the user.

Lyrkan

It seems good to me even though I'm not entirely convinced by the use of rest parameters.

This is not a big deal but:

It would be the first method of the API that works this way
It prevents adding a new parameters without introducting a BC break (but it shouldn't be an issue here since I can't see a reason to add one...)

@weaverryan What do you think?

Lyrkan · 2019-01-02T13:02:39Z

lib/WebpackConfig.js

-        }
-
-        Object.assign(this.externals, externals);
+    addExternals(...externals) {


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)).

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.

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.

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.

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 :)

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:

a string

an array (including an array of arrays)

an object

a function

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.

weaverryan · 2019-01-04T18:36:04Z

test/WebpackConfig.js

@@ -940,24 +940,15 @@ 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' });


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

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

weaverryan · 2019-01-04T18:44:05Z

lib/WebpackConfig.js

-        }
-
-        Object.assign(this.externals, externals);
+    addExternals(...externals) {


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 :)

deAtog · 2019-01-21T16:08:22Z

Closing this pull request as it has been superseded by a more recent version.

…d Ellingsworth, Lyrkan, weaverryan) This PR was merged into the master branch. Discussion ---------- Fix support for Webpack externals. This fixes #471 This is an alternative to #472 which does not modify the public Encore API and requires the user to pass an array if they want to add multiple externals at a time. Commits ------- 58dc86d tweaking example c1fbbf3 Split the addExternals example into two separate examples. c0a8d8d Use {*} instead of {object} for externals parameter of addExternals method. 0e012d3 Fix style issues identified by lint. 5ab6822 Include a regex in the tests of addExternals. a51b35f Update documentation for Encore.addExternals 3d6fb55 Ensure a copy of the externals is returned when generating a Webpack config. 84a0dc0 Store externals in an array. This fixes #471

Store externals in an array. This fixes symfony#471

84a0dc0

Lyrkan requested changes Dec 13, 2018

View reviewed changes

lib/config-generator.js Outdated Show resolved Hide resolved

test/WebpackConfig.js Show resolved Hide resolved

Lyrkan and others added 3 commits December 14, 2018 11:59

Ensure a copy of the externals is returned when generating a Webpack …

3d6fb55

…config. Co-Authored-By: deAtog <[email protected]>

Use ES6 rest parameters for the addExternals method.

13b1780

Update documentation for Encore.addExternals

fc1e1f0

Lyrkan reviewed Jan 2, 2019

View reviewed changes

Use ES6 syntax for adding externals to be consistent with the new API

76e6a95

weaverryan reviewed Jan 4, 2019

View reviewed changes

Include a regex in the tests of addExternals.

4fc6dc3

deAtog mentioned this pull request Jan 15, 2019

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

Merged

deAtog closed this Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

deAtog commented Dec 13, 2018

Uh oh!

Lyrkan left a comment

Uh oh!

Uh oh!

Uh oh!

deAtog commented Dec 17, 2018

Uh oh!

Lyrkan left a comment

Uh oh!

Lyrkan Jan 2, 2019

Uh oh!

deAtog Jan 2, 2019

Uh oh!

Lyrkan Jan 2, 2019

Uh oh!

deAtog Jan 2, 2019

Uh oh!

weaverryan Jan 4, 2019

Uh oh!

deAtog Jan 7, 2019 •

edited

Loading

Uh oh!

weaverryan Jan 4, 2019

Uh oh!

deAtog Jan 7, 2019

Uh oh!

weaverryan Jan 4, 2019

Uh oh!

deAtog commented Jan 21, 2019

Uh oh!

Uh oh!

	/**
	* Allow you to exclude some dependencies from the output bundles.
	*
	* See https://webpack.js.org/configuration/externals/
	*
	* For example:
	*
	* Encore.addExternals({
	* jquery: 'jQuery',
	* react: 'react'
	* })
	*
	* @param {object} externals
	*
	* @returns {Encore}
	*/
	addExternals(externals) {
	webpackConfig.addExternals(externals);

	return this;
	}

Uh oh!

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

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

Uh oh!

Conversation

deAtog commented Dec 13, 2018

Uh oh!

Lyrkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deAtog commented Dec 17, 2018

Uh oh!

Lyrkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deAtog Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deAtog commented Jan 21, 2019

Uh oh!

Uh oh!

deAtog Jan 7, 2019 •

edited

Loading