feat: library target UMD supports names per target by AndersDJohnson · Pull Request #4883 · webpack/webpack

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

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

 		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

@@ -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

+		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

-					(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

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

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

		@@ -0,0 +1 @@
		original No newline at end of file

Uh oh!

Conversation

AndersDJohnson commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsf-clabot commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webpack-bot commented May 17, 2017

Uh oh!

sokra 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!

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!

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!

webpack-bot commented May 29, 2017

Uh oh!

webpack-bot commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra commented May 31, 2017

Uh oh!

AndersDJohnson commented Jun 7, 2017

Uh oh!

AndersDJohnson commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndersDJohnson commented May 17, 2017 •

edited

Loading

jsf-clabot commented May 17, 2017 •

edited

Loading