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

Introduce getOptions #56

Closed
jhnns opened this issue Feb 8, 2017 · 53 comments
Closed

Introduce getOptions #56

jhnns opened this issue Feb 8, 2017 · 53 comments

Comments

@jhnns
Copy link
Member

jhnns commented Feb 8, 2017

The deprecation warning is for webpack loader authors, because calling parseQuery with a non-string value can have bad side-effects (see below). If you're a webpack user, you can set process.traceDeprecation = true in your webpack.config.js to find out which loader is causing this deprecation warning. Please file an issue in the loader's repository.


Starting with webpack 2, it is also possible to pass an object reference from the webpack.config.js to the loader. This is a lot more straight-forward and it allows the use of non-stringifyable values like functions.

For compatibility reasons, this object is still read from the loader context via this.query. Thus, this.query can either be the options object from the webpack.config.js or an actual loader query string. The current implementation of parseQuery just returns this.query if it's not a string.

Unfortunately, this leads to a situation where the loader re-uses the options object across multiple loader invocations. This can be a problem if the loader modifies the object (for instance, to add sane defaults). See webpack-contrib/sass-loader#368 (comment).

Modifying the object was totally fine with webpack 1 since this.query was always a string. Thus, parseQuery would always return a fresh object.

Starting with the next major version of loader-utils, we will unify the way of reading the loader options:

  • We will remove parseQuery and getLoaderConfig
  • We will add getOptions as the only way to get the loader options. It will look like this:
const options = loaderUtils.getOptions(this);

The returned options object is read-only, you should not modify it. This is because getOptions can not make assumptions on how to clone the object correctly.

jhnns added a commit that referenced this issue Feb 9, 2017
This deprecation warning is intended to alarm loader authors that passing an object to parseQuery and then modifying it might lead to unintended behavior.

#56
jhnns added a commit that referenced this issue Feb 10, 2017
This deprecation warning is intended to alarm loader authors that passing an object to parseQuery and then modifying it might lead to unintended behavior.

#56
jhnns added a commit that referenced this issue Feb 10, 2017
This deprecation warning is intended to alarm loader authors that passing an object to parseQuery and then modifying it might lead to unintended behavior.

#56
jhnns added a commit that referenced this issue Feb 18, 2017
@jhnns jhnns mentioned this issue Feb 18, 2017
@blackdynamo
Copy link

Hello, just wondering what I might be doing wrong that's causing this warning. I am trying to follow this issue and understand where things are going and have narrowed down to the following causing the warning vs not.

Case 1: Having an options property causes the warning

  module: {
    rules: [
      {
        test: /\.jsx?$/,
        loader: "babel-loader",
        exclude: /node_modules/,
        options: {
          // https://github.com/babel/babel-loader#options
          cacheDirectory: isDebug,
        }
      },
    ]
  },
[16:48:12] Starting "build"...
[16:48:12] Starting "clean"...
[16:48:13] Finished "clean" after 120 ms
[16:48:13] Starting "bundle"...
(node:85959) DeprecationWarning: loaderUtils.parseQuery() received a non-string value which can be problematic, see https://github.com/webpack/loader-utils/issues/56
parseQuery() will be replaced with getOptions() in the next major version of loader-utils.
Child
    Time: 319ms
         Asset     Size  Chunks             Chunk Names
        api.js  26.5 kB       0  [emitted]  main
    api.js.map  41.2 kB       0  [emitted]  main
Child
    Time: 383ms
             Asset     Size  Chunks             Chunk Names
        service.js  47.8 kB       0  [emitted]  main
    service.js.map  57.5 kB       0  [emitted]  main
[16:48:13] Finished "bundle" after 452 ms
[16:48:13] Finished "build" after 574 ms

Case 2: Removing the options property removes the warning

  module: {
    rules: [
      {
        test: /\.jsx?$/,
        loader: "babel-loader",
        exclude: /node_modules/,
        options: {
          // https://github.com/babel/babel-loader#options
          cacheDirectory: isDebug,
        }
      },
    ]
  },
[16:48:43] Starting "build"...
[16:48:43] Starting "clean"...
[16:48:43] Finished "clean" after 124 ms
[16:48:43] Starting "bundle"...
Child
    Time: 977ms
         Asset     Size  Chunks             Chunk Names
        api.js  26.5 kB       0  [emitted]  main
    api.js.map  41.2 kB       0  [emitted]  main
Child
    Time: 1143ms
             Asset     Size  Chunks             Chunk Names
        service.js  47.8 kB       0  [emitted]  main
    service.js.map  57.5 kB       0  [emitted]  main
[16:48:44] Finished "bundle" after 1215 ms
[16:48:44] Finished "build" after 1342 ms

@elfey
Copy link

elfey commented Feb 21, 2017

My question is, why force every single user of Webpack to update his config when you can just change the underlying code that processes the config to do this automatically?

Right now we're stuck with this deprecation warning and no idea how to fix it.

Edit: this isn't an issue application developers have to worry about, this is for webpack-loader developers.

@fetchTe
Copy link

fetchTe commented Feb 21, 2017

I attempted to update to version 1.0, however, loaderUtils.getOptions(this) throws an error at parseQuery(query) if said query is an empty String at initial validation: query.substr(0, 1) !== "?". I'm assuming this is not by design since it implies a user must specify an option property in the config regardless of whether a user has options to apply, although, maybe I'm interpreting this incorrectly.

@jayullman
Copy link

I am also receiving this warning. I am admittedly new to Webpack and am not sure if this is something that I need to change in my code, or if this is something that will get corrected down the road.

Here is the code that is causing the warning:

module: {
    rules: [
      {
        test: /\.js$/, // include .js files
        exclude: /node_modules/, // exclude any and all files in the node_modules folder
        use:
          {
            loader: "jshint-loader", 
            options: { esversion: 6, camelcase: true, emitErrors: false, failOnHint: false }
          }
      },

Any advice would be greatly appreciated. Thanks!

@johnnyreilly
Copy link

Just tried to upgrade ts-loader to use the new version of loader-utils here: TypeStrong/ts-loader#475

I haven't had chance to look into it but there's issues: https://travis-ci.org/TypeStrong/ts-loader/jobs/203704607

@johnnyreilly
Copy link

BTW in our test packs we have migrated to use options in place of query on the understanding that query was deprecated and options was the new hotness.

@jhnns
Copy link
Member Author

jhnns commented Feb 21, 2017

Sorry for the confusion. This deprecation warning is not for webpack users, but for loader authors. I've added a paragraph in my first comment to clarify that. Use process.traceDeprecation = true in your webpack.config.js to find out which loader is causing this deprecation warning.

We had to go this way because depending on how the loader is using the result of parseQuery it can have bad side-effects, like bloating the bundle (see webpack-contrib/sass-loader#368 (comment)).

@artisin Thanks for pointing that out. I didn't know that there are situations where an empty string is passed to parseQuery. Unfortunately, there were little tests so it was hard to find all use-cases. I'll release a fix in the next hours. @johnnyreilly I think, that fix should also solve your problem?

@johnnyreilly
Copy link

Just having a look at the error output; seeing this:

Error: A valid query string passed to parseQuery should begin with '?'

Will your fix resolve that as well?

@johnnyreilly
Copy link

Actually I've just had a look at the code in the change and spotted a potential problem: https://github.com/webpack/loader-utils/pull/65/files#diff-55c2772ed44f17a0079fc5dabe37be63R6

Shouldn't the getOptions function take into account the options value? It actually does this:

function getOptions(loaderContext) {
	const query = loaderContext.query;
	if(typeof query === "string") {
		return parseQuery(loaderContext.query);
	}
	return query;
}

Should it not use options if it is passed? Something like this:

function getOptions(loaderContext) {
	const query = loaderContext.query;
	if(typeof query === "string") {
		return parseQuery(loaderContext.query);
	}
	return query || loaderContext.options; // If query is undefined then use options
}

@jhnns
Copy link
Member Author

jhnns commented Feb 21, 2017

Will your fix resolve that as well?

Yes

Shouldn't the getOptions function take into account the options value?

No. loaderContext.options are the webpack options from the webpack.config.js (bad naming 😞). They will probably be removed someday because they make it hard to execute loaders in separate processes.

@johnnyreilly
Copy link

Will your fix resolve that as well?

Yes

Awesome.

No. loaderContext.options are the webpack options from the webpack.config.js (bad naming 😞).

huh - does that mean when you pass options in your webpack.config.js that gets turned into query on the other side? Sounds... confusing!

@blackdynamo
Copy link

@jhnns Thanks for the clarification on the warning. I will disregard the warning until it disappears :)

jhnns added a commit that referenced this issue Feb 21, 2017
Unify the handling of unexpected queries:
1. If `this.query` is a string:
	- Tries to parse the query string and returns a new object
	- Throws if it's not a valid query string
2. If `this.query` is object-like, it just returns `this.query`
3. In any other case, it just returns `null`

#56
@jhnns jhnns mentioned this issue Feb 21, 2017
@jhnns
Copy link
Member Author

jhnns commented Feb 21, 2017

Just shipped 1.0.1. Try it out! :)

@johnnyreilly
Copy link

Thanks - just queued a build: https://travis-ci.org/TypeStrong/ts-loader/builds/203855056

@jhnns
Copy link
Member Author

jhnns commented Feb 21, 2017

The build is failing. getOptions() may return null if no options were specified, see README. I've added that later because the handling was not very consistent before that.

NickNaso added a commit to NickNaso/marionette-integrations that referenced this issue Mar 11, 2017
When run build **loader-utils** issued this deprecation message:
```
loader option has been deprecated - replace with "use"
(node:22316) DeprecationWarning: loaderUtils.parseQuery() received a non-string
value which can be problematic
```
So based on **loader-utils** (See: webpack/loader-utils#56)  and **babel-loader** (See: https://github.com/babel/babel-loader#options) documentation.  **options** have been removed as object and used query string instead.
NickNaso added a commit to NickNaso/marionette-integrations that referenced this issue Mar 11, 2017
When run build **loader-utils** issued this deprecation message:
```
loader option has been deprecated - replace with "use"
(node:22316) DeprecationWarning: loaderUtils.parseQuery() received a non-string
value which can be problematic
```
So based on **loader-utils** (See: webpack/loader-utils#56)  and **babel-loader** (See: https://github.com/babel/babel-loader#options) documentation.  **options** have been removed as object and used query string instead.
NickNaso added a commit to NickNaso/marionette-integrations that referenced this issue Mar 11, 2017
When run build **loader-utils** issued this deprecation message:
```
loader option has been deprecated - replace with "use"
(node:22316) DeprecationWarning: loaderUtils.parseQuery() received a non-string
value which can be problematic
```
So based on **loader-utils** (See: webpack/loader-utils#56)  and **babel-loader** (See: https://github.com/babel/babel-loader#options) documentation.  **options** have been removed as object and used query string instead.

Updated **fallbackLoader** to **fallback**
Updated **loader** to **use**
@pla2aroi
Copy link

pla2aroi commented Mar 12, 2017

(node:47141) DeprecationWarning: loaderUtils.parseQuery() received a non-string value which can be problematic, see #56
parseQuery() will be replaced with getOptions() in the next major version of loader-utils.

Solve my problem

Before

   {
        test: /\.js?$/,
        exclude: /node_modules|\.git/,
        loader: 'babel-loader',
        options: {
          babelrc: false,
          presets: ['es2015', 'stage-0', 'react'],
          cacheDirectory: true
        }
      }

After editing, it works.

      {
        test: /\.js?$/,
        exclude: /node_modules|\.git/,
        loader: 'babel-loader?-babelrc,+cacheDirectory,presets[]=es2015,presets[]=stage-0,presets[]=react',
        // options: {
        //   babelrc: false,
        //   presets: ['es2015', 'stage-0', 'react'],
        //   cacheDirectory: true
        // }
      }

@jhnns
Copy link
Member Author

jhnns commented Mar 13, 2017

This is not a good workaround. The babel-loader will soon publish a new version that removes the deprecation warning.

@jagasan
Copy link

jagasan commented Mar 15, 2017

hi i am getting error get method undefined
mounted: function() {
var self = this;
self.axios.get('https://jsonplaceholder.typicode.com/users')
.then(function(response){
console.log('response'+response);
}.bind(this));

    },

@elfey
Copy link

elfey commented Mar 16, 2017

@jagasan that's completely irrelevant to this issue. And if you want my opinion on it, it's likely because axios isn't defined on this.

You're better off asking questions like that on Stack Overflow or on another website.

@Oblik-Design
Copy link

is there a suggested fix ? the thread is really confusing.. I get anxious working with warnings !

@jhnns
Copy link
Member Author

jhnns commented Mar 16, 2017

Have you read the first post in this thread? What is confusing about it?

@hulkish
Copy link

hulkish commented Mar 16, 2017

@jhnns i think he just means that we wanna see this warning go away and would like a way to handle it correctly

@Oblik-Design
Copy link

Oblik-Design commented Mar 16, 2017

you are joking right ? i'm not a dedicated fan to this project, all I aiming for is to not have warnings meant for others... so now, please, I'm pretty sure i'm not the only one in this situation.

@jhnns
Copy link
Member Author

jhnns commented Mar 16, 2017

The deprecation warning is for webpack loader authors, because calling parseQuery with a non-string value can have bad side-effects (see below).

Are you a webpack loader author? If yes, replace parseQuery with getOptions as described in the README

If you're a webpack user, you can set process.traceDeprecation = true in your webpack.config.js to find out which loader is causing this deprecation warning.

Are you a webpack user? Probably yes. Then you need to find out which of your loaders is causing this warning. Copy & paste this line:

process.traceDeprecation = true;

into the first line of your webpack.config.js and take a look at the output in the console. You should see in the stack trace which loader is causing the warning. After you've identified the loader, go to the repository and:

Please file an issue in the loader's repository.

@elfey
Copy link

elfey commented Mar 17, 2017

@Oblik-Design this is open source software... and as you would expect; software evolves and deprecates things. This deprecation needed to be public so that loader authors would hear about it.

And despite this common knowledge, you continue to wine about a harmless deprecation warning and downvote tolerant posts.

  • Wake up.
  • You're not dead.
  • Your project isn't dead.
  • Everything still works as before.
  • If your OCD is kicking in, put a bullet in its head - nobody cares.

These contributors are doing us a favor (so stop wining).

If you felt I was harsh in this criticism of your attitude: good. That's how it was intended.

@jhnns thank you for your work and your tolerance. Maybe it's best this thread is locked.

@Oblik-Design
Copy link

wow, you have read this wrong, english is a third language, must be my fault. no my package does not work properly and adding that process.traceDeprecation = true; did not change a thing.

@jhnns
Copy link
Member Author

jhnns commented Mar 17, 2017

@Oblik-Design if you still don't know what to do, you can safely ignore this deprecation warning. It will disappear over time if you keep your packages up-to-date.

@jhnns
Copy link
Member Author

jhnns commented Mar 17, 2017

This deprecation warning is for webpack loader authors. If you've written your own loader, then please follow the instructions from the first post and the current README.

If you're a webpack user who is using another loader, you can set process.traceDeprecation = true in your webpack.config.js to find out which loader is causing this deprecation warning. Please file an issue in the loader's repository.

If you have no clue what this is all about, you can safely ignore the deprecation warning. It will disappear over time if you keep your packages up-to-date.

@webpack webpack locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests