Skip to content

feat: consolidated sourceMap-related fields in devtool #19485

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: consolidated sourceMap-related fields in devtool #19485

wants to merge 2 commits into from

Conversation

@hai-x
Copy link
Member

@hai-x hai-x commented May 1, 2025 •
edited by evenstensberg
Loading

What kind of change does this PR introduce?

Follows #19389 (comment) Step 1.

Consolidated SourceMap-related fields under devtool to enhance scalability and improve developer experience (DX). Now, devtool can be defined using an object literal, like so:

{
	devtool: {
		"namespace": "webpack",
		"moduleFilenameTemplate":"webpack://[namespace]/[resource-path]",
		"fallbackModuleFilenameTemplate":"webpack://[namespace]/[resource-path]?[hash]",
		"filename": "[file].map[query]",
		"module": true,
		"columns": true,
		"noSources": true,
		"debugIds": true,
		"eval": true // This is a newly introduced option. When set to true, it behaves the same as 'eval-source-map'.
	}
}

Did you add tests for your changes?

Yes

Does this PR introduce a breaking change?

No

What needs to be documented once your changes are merged?

object notation of devtool

hai-x
hai-x commented May 1, 2025
View reviewed changes
lib/config/defaults.js
}
}

F(options, "devtool", () => (development ? "eval" : false));
Copy link
Member Author

@hai-x hai-x May 1, 2025

Choose a reason for hiding this comment

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

We put it after making options.output default b/c we rely on output.uniqueName

@hai-x hai-x marked this pull request as ready for review May 1, 2025 10:30
@hai-x hai-x force-pushed the feat-devtool-object-notation branch from 42ee44e to 9908283 Compare May 1, 2025 12:06
@codspeed-hq
Copy link

codspeed-hq bot commented May 1, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #19485 will degrade performances by 10.48%

Comparing hai-x:feat-devtool-object-notation (7c87b00) with main (19ca741)

Summary

⚡ 80 improvements
❌ 1 regressions
✅ 40 untouched benchmarks
🆕 12 new benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ benchmark "future-defaults", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' 52.3 ms 11.6 ms ×4.5
🆕 benchmark "lodash", scenario '{"name":"mode-development","mode":"development"}' N/A 751.1 ms N/A
🆕 benchmark "lodash", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' N/A 30.1 ms N/A
🆕 benchmark "lodash", scenario '{"name":"mode-production","mode":"production"}' N/A 9.8 s N/A
🆕 benchmark "many-chunks-commonjs", scenario '{"name":"mode-development","mode":"development"}' N/A 247.9 ms N/A
🆕 benchmark "many-chunks-commonjs", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' N/A 61.4 ms N/A
🆕 benchmark "many-chunks-commonjs", scenario '{"name":"mode-production","mode":"production"}' N/A 2 s N/A
🆕 benchmark "many-chunks-esm", scenario '{"name":"mode-development","mode":"development"}' N/A 243.4 ms N/A
🆕 benchmark "many-chunks-esm", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' N/A 61.8 ms N/A
🆕 benchmark "many-chunks-esm", scenario '{"name":"mode-production","mode":"production"}' N/A 2.3 s N/A
🆕 benchmark "many-modules-commonjs", scenario '{"name":"mode-development","mode":"development"}' N/A 306.1 ms N/A
🆕 benchmark "many-modules-commonjs", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' N/A 52.3 ms N/A
🆕 benchmark "many-modules-commonjs", scenario '{"name":"mode-production","mode":"production"}' N/A 2.1 s N/A
❌ benchmark "react", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' 31.3 ms 35 ms -10.48%
⚡ md4 buffer benchmark (size: 10000) 114.1 µs 73 µs +56.26%
⚡ md4 buffer benchmark (size: 100000) 447.6 µs 406.7 µs +10.08%
⚡ md4 buffer benchmark (size: 120) 76 µs 35 µs ×2.2
⚡ md4 buffer benchmark (size: 160) 76 µs 35 µs ×2.2
⚡ md4 buffer benchmark (size: 16366) 138.4 µs 97.4 µs +42.13%
⚡ md4 buffer benchmark (size: 16368) 138.6 µs 97.4 µs +42.2%
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@hai-x hai-x force-pushed the feat-devtool-object-notation branch 2 times, most recently from 27ec59a to 2ed2eac Compare May 7, 2025 14:43
alexander-akait
alexander-akait reviewed May 21, 2025
View reviewed changes
lib/WebpackOptionsApply.js Outdated
const cheap = devtool.type.includes("cheap");
const moduleMaps = devtool.type.includes("module");
const noSources = devtool.type.includes("nosources");
const debugIds = devtool.type.includes("debugids");
Copy link
Member

@alexander-akait alexander-akait May 21, 2025

Choose a reason for hiding this comment

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

Ideally we should do it in normalization.js, so in default.js we will already have object, but I am afraid it can be breaking change, because some developers can rely on output.devtool as a string.

There is a good question should we want to support devtool: string in future, if yes - code looks good, if no - we need to add TODO.

I'm actually in favor of supporting the string value in the future.

Copy link
Member Author

@hai-x hai-x May 22, 2025

Choose a reason for hiding this comment

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

I'm actually in favor of supporting the string value in the future.

Me too. I think it's not too bad to still supporting devtool: string. So user can easily setting when development and as far as I know, if need to generate production sourcemaps they usually use plugin.

alexander-akait
alexander-akait reviewed May 21, 2025
View reviewed changes
lib/WebpackOptionsApply.js
namespace: options.output.devtoolNamespace,
namespace,
debugIds
}).apply(compiler);
Copy link
Member

@alexander-akait alexander-akait May 21, 2025

Choose a reason for hiding this comment

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

So we don't allow to provide columns/debugIds/etc values in the object notation or you want to finish it after this PR?

Copy link
Member Author

@hai-x hai-x May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, columns and debugIds depend on the devtool setting (i.e., devtool.includes("cheap") / "debugids").

Yeah, separating them into a dedicated field is a good idea - but should we still support specifying them via string literals or by devtool.type (e.g., cheap-xxx, debugids-xxx)?

Copy link
Member

@alexander-akait alexander-akait May 22, 2025

Choose a reason for hiding this comment

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

I think it makes sense only for an object notation, we already have string values which cover every option

@hai-x hai-x force-pushed the feat-devtool-object-notation branch 3 times, most recently from 172c0bf to d98ffbf Compare May 23, 2025 11:19
@hai-x hai-x force-pushed the feat-devtool-object-notation branch from d98ffbf to 6012ee0 Compare May 23, 2025 11:20
alexander-akait
alexander-akait reviewed Jun 2, 2025
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, can we add a couple simple test cases?

evenstensberg
evenstensberg reviewed Jun 26, 2025
View reviewed changes
evenstensberg
evenstensberg approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

@evenstensberg evenstensberg evenstensberg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hai-x @alexander-akait @evenstensberg