adding `output.devtoolNamespace` option #5844

MagicDuck · 2017-10-17T23:55:09Z

What kind of change does this PR introduce?
feature

Did you add tests for your changes?
yes

If relevant, link to documentation update:

webpack/webpack.js.org#1653

Summary

When loading multiple libraries built with webpack, you can run into
collisions of the sourcemap file paths. For examle, both have
"webpack:///src/index.js".

This change addresses the problem by introducing a new output option
output.devtoolNamespace which defaults to output.library when
not specified. The defaults moduleFilenameTemplates in all the
sourcemap plugins have been modified to start with:
"webpack://[namespace]/...", where [namespace] will be replaced by
the output.devtoolNamespace.

Notice that there are only two slashes following "webpack:" now.
This is to make it behave just as before when not building with a
namespace. When building with a namespace you only get the two
slashes, but from what I've seen the chrome dev tools only care
about the first 2 slashes anyways.

Discussed with sokra here:
#5767

Does this PR introduce a breaking change?

No

Other information

jsf-clabot · 2017-10-17T23:55:10Z

All committers have signed the CLA.

When loading multiple libraries built with webpack, you can run into collisions of the sourcemap file paths. For examle, both have "webpack:///src/index.js". This change addresses the problem by introducing a new output option `output.devtoolNamespace` which defaults to `output.library` when not specified. The defaults moduleFilenameTemplates in all the sourcemap plugins have been modified to start with: "webpack://[namespace]/...", where [namespace] will be replaced by the `output.devtoolNamespace`. Notice that there are only two slashes following "webpack:" now. This is to make it behave just as before when not building with a namespace. When building with a namespace you only get the two slashes, but from what I've seen the chrome dev tools only care about the first 2 slashes anyways. Discussed with sokra here: webpack#5767

sokra · 2017-10-18T13:28:17Z

lib/ModuleFilenameHelpers.js

@@ -160,3 +165,7 @@ ModuleFilenameHelpers.matchObject = function matchObject(obj, str) {
 		if(ModuleFilenameHelpers.matchPart(str, obj.exclude)) return false;
 	return true;
 };
+
+ModuleFilenameHelpers.setModuleNamespace = function setModuleNamespace(moduleNamespace) {
+	this._moduleNamespace = moduleNamespace;


Don't use static variables.

Instead pass the namespace to the createFilename method.

createFilename(module, options, requestShortener)

options: { namespace, moduleFilenameTemplate }

Also add a backward-compatibility layer when typeof options === "string"

As it's now refactored to use options you can remove this method.

sokra · 2017-10-18T13:29:17Z

lib/WebpackOptionsApply.js

+		if(!options.output.devtoolNamespace) {
+			options.output.devtoolNamespace = options.output.library;
+		}
+		ModuleFilenameHelpers.setModuleNamespace(options.output.devtoolNamespace);


Pass namespace to the SourceMapDevtoolPlugin (next section) instead. Similar to devtoolModuleFilenameTemplate

webpack-bot · 2017-10-18T22:25:21Z

@MagicDuck Thanks for your update.

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

@sokra Please review the new changes.

webpack-bot · 2017-10-19T00:21:55Z

Hi @MagicDuck.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own master branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

MagicDuck · 2017-10-19T04:19:06Z

I'll make it default to empty string in the webpack options defaulter to pass the tests.

sokra · 2017-10-19T09:15:49Z

lib/EvalDevToolModulePlugin.js

@@ -14,7 +14,7 @@ class EvalDevToolModulePlugin {

 	apply(compiler) {
 		compiler.plugin("compilation", (compilation) => {
-			compilation.moduleTemplate.apply(new EvalDevToolModuleTemplatePlugin(this.sourceUrlComment, this.moduleFilenameTemplate));
+			compilation.moduleTemplate.apply(new EvalDevToolModuleTemplatePlugin(this.sourceUrlComment, this.moduleFilenameTemplate, compiler.options.output.devtoolNamespace));


change the EvalDevToolModulePlugin constructor to EvalDevToolModulePlugin(options) with options including sourceUrlComment, moduleFilenameTemplate and namespace.

Do you guys have any standard for documenting what the options are?
There is no way to guess from seeing something like:

constructor(options) { this.options = options; }

Should I add some jsdoc comments?

or do you completely destructure all options in the constructor?

sokra · 2017-10-19T09:16:04Z

lib/EvalDevToolModuleTemplatePlugin.js

@@ -8,15 +8,19 @@ const RawSource = require("webpack-sources").RawSource;
 const ModuleFilenameHelpers = require("./ModuleFilenameHelpers");

 class EvalDevToolModuleTemplatePlugin {
-	constructor(sourceUrlComment, moduleFilenameTemplate) {
+	constructor(sourceUrlComment, moduleFilenameTemplate, namespace) {


constructor(options)

sokra · 2017-10-19T09:16:45Z

lib/EvalSourceMapDevToolModuleTemplatePlugin.js

@@ -11,7 +11,8 @@ class EvalSourceMapDevToolModuleTemplatePlugin {
 	constructor(compilation, options) {
 		this.compilation = compilation;
 		this.sourceMapComment = options.append || "//# sourceURL=[module]\n//# sourceMappingURL=[url]";
-		this.moduleFilenameTemplate = options.moduleFilenameTemplate || "webpack:///[resource-path]?[hash]";
+		this.moduleFilenameTemplate = options.moduleFilenameTemplate || "webpack://[namespace]/[resource-path]?[hash]";
+		this.namespace = compilation.options.output.devtoolNamespace;


add namespace to options

sokra · 2017-10-19T09:19:47Z

lib/ModuleFilenameHelpers.js

@@ -48,7 +50,17 @@ function asRegExp(test) {
 	return test;
 }

-ModuleFilenameHelpers.createFilename = function createFilename(module, moduleFilenameTemplate, requestShortener) {
+ModuleFilenameHelpers.createFilename = function createFilename(module, options, requestShortener) {
+	let opts;


You could do something like:

const opts = Object.assign({ namespace: "", moduleFilenameTemplate: "" }, typeof options === "object" ? options : { moduleFilenameTemplate: options });

Using Object.assign() can have weird consequences sometimes for your defaults:

Object.assign({foo: "default"}, {foo: undefined}) --> {foo: undefined}

Can we use extend lib instead? (https://www.npmjs.com/package/extend)

var extend = require("extend") extend({foo: "default"}, {foo: undefined}) ---> {foo: "default"}

https://runkit.com/magicduck/59e8fd082fe1da001215d79f

sokra · 2017-10-19T09:20:23Z

lib/ModuleFilenameHelpers.js

+		opts = options;
+	} else {
+		opts = {
+			moduleFilenameTemplate: arguments[1],


In general try to avoid using arguments.

sokra · 2017-10-19T09:21:06Z

lib/ModuleFilenameHelpers.js

@@ -160,3 +165,7 @@ ModuleFilenameHelpers.matchObject = function matchObject(obj, str) {
 		if(ModuleFilenameHelpers.matchPart(str, obj.exclude)) return false;
 	return true;
 };
+
+ModuleFilenameHelpers.setModuleNamespace = function setModuleNamespace(moduleNamespace) {
+	this._moduleNamespace = moduleNamespace;


As it's now refactored to use options you can remove this method.

sokra · 2017-10-19T09:22:37Z

lib/SourceMapDevToolPlugin.js

@@ -72,6 +72,7 @@ class SourceMapDevToolPlugin {
 		const sourceMappingURLComment = this.sourceMappingURLComment;
 		const moduleFilenameTemplate = this.moduleFilenameTemplate;
 		const fallbackModuleFilenameTemplate = this.fallbackModuleFilenameTemplate;
+		const namespace = compiler.options.output.devtoolNamespace;


Don't use compiler.options. The plugin could be used directly without using devtool.

Pass namespace via the options object in the constructor.

sokra · 2017-10-19T09:23:36Z

lib/WebpackOptionsApply.js

@@ -193,6 +193,11 @@ class WebpackOptionsApply extends OptionsApply {
 			ExternalsPlugin = require("./ExternalsPlugin");
 			compiler.apply(new ExternalsPlugin(options.output.libraryTarget, options.externals));
 		}
+
+		if(!options.output.devtoolNamespace) {
+			options.output.devtoolNamespace = options.output.library;


This should not happen here. You can do this in WebpackOptionsDefaulter.

MagicDuck · 2017-10-20T19:53:14Z

@sokra when you get a chance, let me know what approach I should follow regarding documenting and defaulting options (see my comments)

webpack-bot · 2017-10-20T20:14:02Z

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

sokra · 2017-11-16T17:01:25Z

Thanks

MagicDuck · 2017-11-16T18:37:43Z

Yay, thanks!

MagicDuck mentioned this pull request Oct 17, 2017

sourcemap collision when building as a library #5767

Closed

webpack-bot added the PR: CI-ok label Oct 18, 2017

MagicDuck force-pushed the master branch from 0b44ed8 to fa8b790 Compare October 18, 2017 00:13

MagicDuck force-pushed the master branch from fa8b790 to f925032 Compare October 18, 2017 00:14

webpack-bot added the PR: unreviewed label Oct 18, 2017

sokra requested changes Oct 18, 2017

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Oct 18, 2017

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

passing devtoolNamespace through instead of setting it statically

72b28aa

MagicDuck force-pushed the master branch from 363884a to 72b28aa Compare October 18, 2017 22:30

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Oct 18, 2017

sokra added the PR: breaking change label Oct 19, 2017

sokra requested changes Oct 19, 2017

View reviewed changes

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

passing namespace argument to contructors of dev tool plugins

68acd41

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

fixing code style issue

f0b81cb

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Oct 20, 2017

sokra changed the base branch from master to next November 6, 2017 16:55

webpack-bot added the PR: next label Nov 6, 2017

sokra closed this Nov 6, 2017

sokra reopened this Nov 6, 2017

webpack-bot added PR: conflict and removed PR: CI-ok labels Nov 9, 2017

sokra added 2 commits November 11, 2017 19:00

Merge branch 'next' into master

4597805

Merge branch 'next' into master

982e44f

sokra closed this Nov 16, 2017

sokra reopened this Nov 16, 2017

sokra approved these changes Nov 16, 2017

View reviewed changes

webpack-bot added PR: CI-ok PR: reviewed-approved and removed PR: conflict PR: review-outdated labels Nov 16, 2017

sokra merged commit 973253b into webpack:next Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding `output.devtoolNamespace` option #5844

adding `output.devtoolNamespace` option #5844

MagicDuck commented Oct 17, 2017

jsf-clabot commented Oct 17, 2017 •

edited

sokra Oct 18, 2017

sokra Oct 19, 2017

sokra Oct 18, 2017

webpack-bot commented Oct 18, 2017

webpack-bot commented Oct 19, 2017

MagicDuck commented Oct 19, 2017

sokra Oct 19, 2017

MagicDuck Oct 19, 2017

MagicDuck Oct 19, 2017

sokra Oct 19, 2017

sokra Oct 19, 2017

sokra Oct 19, 2017

MagicDuck Oct 19, 2017

sokra Oct 19, 2017

sokra Oct 19, 2017

sokra Oct 19, 2017

sokra Oct 19, 2017

MagicDuck commented Oct 20, 2017

webpack-bot commented Oct 20, 2017

sokra commented Nov 16, 2017

MagicDuck commented Nov 16, 2017

adding output.devtoolNamespace option #5844

adding output.devtoolNamespace option #5844

Conversation

MagicDuck commented Oct 17, 2017

jsf-clabot commented Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented Oct 18, 2017

webpack-bot commented Oct 19, 2017

MagicDuck commented Oct 19, 2017

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

MagicDuck commented Oct 20, 2017

webpack-bot commented Oct 20, 2017

sokra commented Nov 16, 2017

MagicDuck commented Nov 16, 2017

adding `output.devtoolNamespace` option #5844

adding `output.devtoolNamespace` option #5844

jsf-clabot commented Oct 17, 2017 •

edited