feat: getOptions util for loader by alexander-akait · Pull Request #10017 · webpack/webpack

alexander-akait · 2019-11-21T13:38:33Z

New getOptions util for loader (part of migration loader-utils into core)

What kind of change does this PR introduce?

feature

Did you add tests for your changes?

Yes, after feedback i will add more tests

Does this PR introduce a breaking change?

No

What needs to be documented once your changes are merged?

getOptions now in core as part of loader context and have this function signature -> this.getOptions(schema?: JSONSchema).

It differs from the getOptions from schema-utils. Upgrading a loader to the new getOptions is a breaking change for these reasons:

It's only supported in webpack 5.
Instead of JSON5 it only support JSON as query string ?{arg:true} -> ?{"arg":true}
- Hopefully nobody writes inline loader query strings this way anyway.
- Using JSON should considered as deprecated and not be promoted in the loader documentation
loader-utils has some special behavior for parsing query strings (true false and null won't be parsed as string but as primitive value). This is no longer the case for this.getOptions which uses native querystring parsing (from node.js).
- Special handing can be added in loader code after getting the options
Schema is optional, but when doing a breaking change anyway, please add schema validation for your options.
- The title field in the schema can be used to customize the validation error message: e. g. "title": "My Loader ooooptions" will display errors like Invalid ooooptions object. My Loader has been initialised using an ooooptions object that does not match the API schema. - ooooptions.foo.bar.baz should be a string.

webpack-bot · 2019-11-21T13:38:37Z

For maintainers only:

This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
This needs to be backported to webpack 4 (issue will be created when merged)

alexander-akait · 2019-11-21T13:39:19Z

+
+					if (query.substr(0, 1) === "{" && query.substr(-1) === "}") {
+						try {
+							options = parseJson(query);


Should we use json5 here? Because previously implementation used json5 https://github.com/webpack/loader-utils/blob/master/lib/parseQuery.js#L3

alexander-akait · 2019-11-21T13:40:28Z

+					query = query.substr(1);
+
+					// Allow to use `?foo=bar` in `options`
+					if (query.substr(0, 1) === "?") {


Should we support options: "?foo=bar"? I can update schema to disable because query contains ??foo=bar

double ?? has a special meaning anyway so loader??foo=bar is not legal.

I think we should deprecate support for options: "string" at all (in config).

No we don't need to support options: "?foo=bar".

alexander-akait · 2019-11-21T13:40:53Z

+							throw new WebpackError(`Cannot parse query string: ${e.message}`);
+						}
+					} else {
+						options = querystring.parse(query);


Should we support specialValues https://github.com/webpack/loader-utils/blob/master/lib/parseQuery.js#L5?

webpack-bot · 2019-11-21T14:01:57Z

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

sokra · 2019-11-25T15:00:34Z

+				}
+
+				validateOptions(schema, options, {
+					name: loaderName || "Unknown Loader",


I think its fine to use getCurrentLoaderName() here and omit the loaderName argument.

sokra · 2019-11-25T15:11:22Z

+					query = query.substr(1);
+
+					// Allow to use `?foo=bar` in `options`
+					if (query.substr(0, 1) === "?") {


double ?? has a special meaning anyway so loader??foo=bar is not legal.

I think we should deprecate support for options: "string" at all (in config).

sokra · 2019-11-25T15:12:28Z

+					query = query.substr(1);
+
+					// Allow to use `?foo=bar` in `options`
+					if (query.substr(0, 1) === "?") {


No we don't need to support options: "?foo=bar".

sokra · 2019-11-25T15:17:07Z

 		const loaderContext = {
 			version: 2,
+			getOptions: (loaderName, schema) => {
+				const loader = loaderContext.loaders[loaderContext.loaderIndex];


Suggested change

const loader = loaderContext.loaders[loaderContext.loaderIndex];

const loader = this.getCurrentLoader(loaderContext);

alexander-akait · 2020-01-16T13:55:43Z

-				});
+				if (schema) {
+					validateOptions(schema, options, {
+						name: getCurrentLoaderName(),


What if i need to setup name for a loader? schema-utils prefer the name argument under name in schema

Yep, fix that. Now it uses schema.title, but falls back to Loader and options (instead of the default Object and configuration).

Maybe fallbackName and fallbackBaseDataPath in the schema-utils API would be nice.

sokra · 2020-01-17T11:19:47Z

-				});
+				if (schema) {
+					validateOptions(schema, options, {
+						name: getCurrentLoaderName(),


Yep, fix that. Now it uses schema.title, but falls back to Loader and options (instead of the default Object and configuration).

Maybe fallbackName and fallbackBaseDataPath in the schema-utils API would be nice.

alexander-akait · 2020-01-17T11:36:15Z

-						name: getCurrentLoaderName(),
-						baseDataPath: "options"
+						name,
+						baseDataPath


What do you think about change behavior (prefer schema title over arguments) in next schema-utils release?

I think both behaviors make sense for different use cases.

webpack-bot · 2020-01-17T11:36:20Z

The minimum test ratio has been reached. Thanks!

sokra · 2020-01-17T16:28:04Z

Thanks

webpack-bot · 2020-01-17T16:28:05Z

I've created an issue to document this in webpack/webpack.js.org.

AshifMohammad · 2021-07-04T08:42:39Z

getOption ?{arg:true} supports optionally however should we make sure that user knows that he has art also i agree we should not support option?foo=bar 💯

webpack-bot added the PR: unreviewed label Nov 21, 2019

alexander-akait commented Nov 21, 2019

View reviewed changes

webpack-bot added the PR: CI-ok label Nov 21, 2019

sokra requested changes Nov 25, 2019

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Nov 25, 2019

alexander-akait and others added 2 commits January 16, 2020 11:43

feat: getOptions util for loader

4e808cb

improve code, remove unneeded old stuff

f9846f1

sokra force-pushed the feat-getOptions-util-for-loader branch from 561fc0a to f9846f1 Compare January 16, 2020 13:49

alexander-akait commented Jan 16, 2020

View reviewed changes

webpack-bot added the PR: tests-needed label Jan 16, 2020

test errors message, allow custom names in schema

d673e41

sokra approved these changes Jan 17, 2020

View reviewed changes

webpack-bot added the PR: reviewed-approved label Jan 17, 2020

alexander-akait commented Jan 17, 2020

View reviewed changes

webpack-bot removed the PR: tests-needed label Jan 17, 2020

sokra merged commit bd08639 into master Jan 17, 2020

sokra deleted the feat-getOptions-util-for-loader branch January 17, 2020 16:28

webpack-bot mentioned this pull request Jan 17, 2020

Document webpack change: feat: getOptions util for loader webpack/webpack.js.org#3498

Closed

ef4 mentioned this pull request Dec 8, 2020

Webpack 5's "getOptions" is missing webpack/thread-loader#106

Closed

Randy808 mentioned this pull request May 30, 2021

Feature proposal: Add getOptions to loaderContext inside runLoaders webpack/loader-runner#52

Closed

webpack-bot added the PR: non-main label Jul 4, 2021

	const loader = loaderContext.loaders[loaderContext.loaderIndex];
	const loader = this.getCurrentLoader(loaderContext);

Uh oh!

Conversation

alexander-akait commented Nov 21, 2019 • edited by sokra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webpack-bot commented Nov 21, 2019 • edited by alexander-akait Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Nov 21, 2019

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!

alexander-akait Jan 16, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

webpack-bot commented Jan 17, 2020

Uh oh!

sokra commented Jan 17, 2020

Uh oh!

webpack-bot commented Jan 17, 2020

Uh oh!

AshifMohammad commented Jul 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexander-akait commented Nov 21, 2019 •

edited by sokra

Loading

webpack-bot commented Nov 21, 2019 •

edited by alexander-akait

Loading

alexander-akait Jan 16, 2020 •

edited

Loading