feat: library target UMD supports names per target #4883

AndersDJohnson · 2017-05-17T04:48:33Z

What kind of change does this PR introduce?

feature

Did you add tests for your changes?

Yes, I updated the tests for LibraryTemplatePlugin. There didn't seem to be any for UmdMainTemplatePlugin yet - not sure if that's intentional, but if not, I could look into adding.

If relevant, link to documentation update:

I'll take the time to add documentation after getting approval for this feature.

Summary

Currently, the output.libraryTarget of 'umd' only supports a single name (output.library), whereas the user may instead wish to specify a different name for each target type (CommonJS, AMD, and global), e.g., to use kebab-case for AMD and CommonJS but camelCase for global.

Does this PR introduce a breaking change?

No.

Other information

Feel free to ask.

jsf-clabot · 2017-05-17T04:48:35Z

All committers have signed the CLA.

webpack-bot · 2017-05-17T05:01:46Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

sokra

A testcase in test/configCases would be great too to test the configuration schema

sokra · 2017-05-17T06:03:06Z

lib/UmdMainTemplatePlugin.js

@@ -163,12 +173,12 @@ class UmdMainTemplatePlugin {
 				"})(this, function(" + externalsArguments(externals) + ") {\nreturn ", "webpack/universalModuleDefinition"), source, ";\n})");
 		}.bind(this));
 		mainTemplate.plugin("global-hash-paths", function(paths) {
-			if(this.name) paths = paths.concat(this.name);
+			if(this.names.root) paths = paths.concat(this.names.root);


Could you add all names here?

@sokra Is it okay if names are repeated in paths, or do we need to dedupe first?

That's ok. They should just contribute all to the hash.

@sokra Again here, the paths have been added for all names.

sokra · 2017-05-17T06:03:36Z

lib/UmdMainTemplatePlugin.js

 			return paths;
 		}.bind(this));
 		mainTemplate.plugin("hash", function(hash) {
 			hash.update("umd");
-			hash.update(`${this.name}`);
+			hash.update(`${this.names.root}`);


here also all names

@sokra Again, is it okay if names are repeated here, or do we need to dedupe first?

yep, it's ok

@sokra The hash has been updated with all names.

sokra · 2017-05-17T06:04:19Z

test/fixtures/temp-1000/file2.js

@@ -0,0 +1 @@
+original


these files should not be commited (temp)

@sokra Oops, thanks. Any reason not to add a pattern to .gitignore for these? As well as test/statsCases/*/actual.txt?

sokra · 2017-05-17T06:06:31Z

lib/UmdMainTemplatePlugin.js

+		if(typeof name === "object" && !Array.isArray(name)) {
+			this.name = name.root || name.amd || name.commonjs;
+			this.names = name;
+		} else {


does the schema allow an object as output.libraryName?

@sokra Good point. Updated schema and added test.

sokra · 2017-05-17T06:10:46Z

lib/UmdMainTemplatePlugin.js

-					(this.name && this.namedDefine === true ?
-						"		define(" + libraryName(this.name) + ", [], " + amdFactory + ");\n" :
+					(this.names.amd && this.namedDefine === true ?
+						"		define(" + libraryName(this.names.amd) + ", [], " + amdFactory + ");\n" :


this line is not tested, if you want to you can add a test, but it's not required.

@sokra Thanks, I've added tests for namedDefine being true and false. I'm not sure why it was a string value before, as I'm not sure that's a valid value per the docs - I'd only copied that from other test cases.

@sokra Not sure how to get that line covered. Looks like it wasn't before either, and this PR now improves coverage overall. https://codecov.io/gh/webpack/webpack/pull/4883/changes

webpack-bot · 2017-05-29T18:36:31Z

@AndersDJohnson Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

webpack-bot · 2017-05-30T20:34:04Z

It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff).

A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future).

@AndersDJohnson Please check if this is appliable to your PR and if you can add more test cases.

Read the test readme for details how to write test cases.

sokra · 2017-05-31T08:21:01Z

lib/UmdMainTemplatePlugin.js

@@ -163,12 +173,12 @@ class UmdMainTemplatePlugin {
 				"})(this, function(" + externalsArguments(externals) + ") {\nreturn ", "webpack/universalModuleDefinition"), source, ";\n})");
 		}.bind(this));
 		mainTemplate.plugin("global-hash-paths", function(paths) {
-			if(this.name) paths = paths.concat(this.name);
+			if(this.names.root) paths = paths.concat(this.names.root);


That's ok. They should just contribute all to the hash.

sokra · 2017-05-31T08:21:07Z

lib/UmdMainTemplatePlugin.js

 			return paths;
 		}.bind(this));
 		mainTemplate.plugin("hash", function(hash) {
 			hash.update("umd");
-			hash.update(`${this.name}`);
+			hash.update(`${this.names.root}`);


yep, it's ok

sokra · 2017-05-31T08:23:47Z

Could you sign the CLA, so we can merge it?

AndersDJohnson · 2017-06-07T23:01:25Z

@sokra CLA has been signed.

AndersDJohnson · 2017-06-21T21:16:47Z

@sokra Thanks for merging.

webpack-bot added the PR: unreviewed label May 17, 2017

webpack-bot added PR: CI-ok PR: tests-needed labels May 17, 2017

sokra requested changes May 17, 2017

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels May 17, 2017

sokra reviewed May 17, 2017

View reviewed changes

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels May 29, 2017

AndersDJohnson added 3 commits May 29, 2017 13:49

feat: library target UMD supports names per target

0ab7b1a

fix: update schema to support library as object

2ec4b5c

chore: remove temp files

d87fb0e

AndersDJohnson force-pushed the library-target-umd-names-per-target branch from 5e8dcd6 to d87fb0e Compare May 29, 2017 18:49

AndersDJohnson added 2 commits May 29, 2017 14:06

test: add for namedDefine true and false

369efb3

fix: update hashes for all library names

913b18b

sokra approved these changes May 31, 2017

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels May 31, 2017

sokra merged commit 3e3b748 into webpack:master Jun 21, 2017

AndersDJohnson deleted the library-target-umd-names-per-target branch June 21, 2017 21:36

renovate-tests mentioned this pull request Jul 7, 2017

Update dependency webpack to v3 renovate-tests/discover-package-files#13

Open

This was referenced Jul 7, 2017

⬆️ webpack -> ~3.1.0 homezen/hz-test-helpers#54

Merged

⬆️ webpack @ ~3.1.0 homezen/redux-variants#164

Merged

simon04 mentioned this pull request Jul 7, 2017

docs(output): library names for UMD since 3.1.0 webpack/webpack.js.org#1389

Merged

renovate bot mentioned this pull request Jul 7, 2017

Update dependency webpack to version 3.x algolia/react-element-to-jsx-string#105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: library target UMD supports names per target #4883

feat: library target UMD supports names per target #4883

AndersDJohnson commented May 17, 2017 •

edited

jsf-clabot commented May 17, 2017 •

edited

webpack-bot commented May 17, 2017

sokra left a comment

sokra May 17, 2017

AndersDJohnson May 29, 2017

sokra May 31, 2017

AndersDJohnson Jun 7, 2017

sokra May 17, 2017

AndersDJohnson May 29, 2017

sokra May 31, 2017

AndersDJohnson Jun 7, 2017

sokra May 17, 2017

AndersDJohnson May 29, 2017

sokra May 17, 2017

AndersDJohnson May 29, 2017

sokra May 17, 2017

AndersDJohnson May 29, 2017

AndersDJohnson May 29, 2017

webpack-bot commented May 29, 2017

webpack-bot commented May 30, 2017

sokra May 31, 2017

sokra May 31, 2017

sokra commented May 31, 2017

AndersDJohnson commented Jun 7, 2017

AndersDJohnson commented Jun 21, 2017

		@@ -0,0 +1 @@
		original

feat: library target UMD supports names per target #4883

feat: library target UMD supports names per target #4883

Conversation

AndersDJohnson commented May 17, 2017 • edited

jsf-clabot commented May 17, 2017 • edited

webpack-bot commented May 17, 2017

sokra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented May 29, 2017

webpack-bot commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sokra commented May 31, 2017

AndersDJohnson commented Jun 7, 2017

AndersDJohnson commented Jun 21, 2017

AndersDJohnson commented May 17, 2017 •

edited

jsf-clabot commented May 17, 2017 •

edited