Skip to content
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.

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

feat: getOptions util for loader #10017

Merged
merged 3 commits into from Jan 17, 2020
Merged

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Nov 21, 2019

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
Copy link
Contributor

webpack-bot commented Nov 21, 2019

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)


if (query.substr(0, 1) === "{" && query.substr(-1) === "}") {
try {
options = parseJson(query);
Copy link
Member Author

@alexander-akait alexander-akait Nov 21, 2019

Choose a reason for hiding this comment

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

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

query = query.substr(1);

// Allow to use `?foo=bar` in `options`
if (query.substr(0, 1) === "?") {
Copy link
Member Author

@alexander-akait alexander-akait Nov 21, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@sokra sokra Nov 25, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@sokra sokra Nov 25, 2019

Choose a reason for hiding this comment

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

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

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

@alexander-akait alexander-akait Nov 21, 2019

Choose a reason for hiding this comment

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

@webpack-bot
Copy link
Contributor

webpack-bot commented Nov 21, 2019

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

}

validateOptions(schema, options, {
name: loaderName || "Unknown Loader",
Copy link
Member

@sokra sokra Nov 25, 2019

Choose a reason for hiding this comment

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

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

query = query.substr(1);

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

@sokra sokra Nov 25, 2019

Choose a reason for hiding this comment

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

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

query = query.substr(1);

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

@sokra sokra Nov 25, 2019

Choose a reason for hiding this comment

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

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

@@ -327,6 +330,50 @@ class NormalModule extends Module {
};
const loaderContext = {
version: 2,
getOptions: (loaderName, schema) => {
const loader = loaderContext.loaders[loaderContext.loaderIndex];
Copy link
Member

@sokra sokra Nov 25, 2019

Choose a reason for hiding this comment

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

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

@sokra sokra force-pushed the feat-getOptions-util-for-loader branch from 561fc0a to f9846f1 Compare Jan 16, 2020
});
if (schema) {
validateOptions(schema, options, {
name: getCurrentLoaderName(),
Copy link
Member Author

@alexander-akait alexander-akait Jan 16, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@sokra sokra Jan 17, 2020

Choose a reason for hiding this comment

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

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
sokra approved these changes Jan 17, 2020
});
if (schema) {
validateOptions(schema, options, {
name: getCurrentLoaderName(),
Copy link
Member

@sokra sokra Jan 17, 2020

Choose a reason for hiding this comment

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

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.

name: getCurrentLoaderName(),
baseDataPath: "options"
name,
baseDataPath
Copy link
Member Author

@alexander-akait alexander-akait Jan 17, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@sokra sokra Jan 17, 2020

Choose a reason for hiding this comment

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

I think both behaviors make sense for different use cases.

@webpack-bot
Copy link
Contributor

webpack-bot commented Jan 17, 2020

The minimum test ratio has been reached. Thanks!

@sokra sokra merged commit bd08639 into master Jan 17, 2020
@sokra sokra deleted the feat-getOptions-util-for-loader branch Jan 17, 2020
@sokra
Copy link
Member

sokra commented Jan 17, 2020

Thanks

@webpack-bot
Copy link
Contributor

webpack-bot commented Jan 17, 2020

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

@AshifMohammad
Copy link

AshifMohammad commented Jul 4, 2021

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 💯

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

Successfully merging this pull request may close these issues.

None yet

4 participants