-
Notifications
You must be signed in to change notification settings - Fork 914
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
[Fix for Webpack 2 Beta 24+] Get loader options from query instead of raw this.options #361
[Fix for Webpack 2 Beta 24+] Get loader options from query instead of raw this.options #361
Conversation
…an be passed an object and this.options is mounted to this.query
var query = loaderUtils.parseQuery(this.query) | ||
var options = this.options.vue || query || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should
- keep
this.vue
for current usage compatibility - prioritize query first
So perhaps query || this.vue || this.options.vue || {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, will do. I believe there was a reason I couldn't keep query
at the front, I'll check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yyx990803 Alright, so the issue is that query is an empty object by default, and therefore needs to be at the end of the list.
Perhaps we should try merging this.options.vue
and this.vue
into query?
As of the new commit, it tests the this.query
string for emptiness, in which case it skips over query
to this.vue
and so on. I'm not a huge fan of this as it seems hackish, but it should work for now.
Ideally query and options would be combined into the same object, as in Webpack 2. Any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, merging makes sense. So something like Object.assign({}, this.options.vue, this.vue, query)
should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge it now and tweak - thanks for the effort!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! Thanks for the merge! Great work on Vue. :)
… raw this.options (#361) * Get options from query instead of this.options. In Webpack 2, query can be passed an object and this.options is mounted to this.query * Prioritize Query, re-add this.vue
… raw this.options (vuejs#361) * Get options from query instead of this.options. In Webpack 2, query can be passed an object and this.options is mounted to this.query * Prioritize Query, re-add this.vue
The current way to handle Webpack 2 Beta 24+ involves the use of
LoaderOptionsPlugin
. This is non-ideal, asLoaderOptionsPlugin
actually manglesthis.options
in loaders to refer to its configuration instead of what webpack normally passes. Because of this,LoaderOptionsPlugin
should be used only as a stopgap until a fix is introduced to make the loader compatible with Webpack 2 Beta 24+ by loading its options fromthis.query
instead ofthis.options.x
.Current Usage (
LoaderOptionsPlugin
):Recommended Usage (Works after this patch) (Current and Webpack 1.x usages still work and can mix & match):
I hope this is of use. The removal of references to
this.options
may cause breakages in Webpack 1.x, though all tests are passing. If someone wants to confirm this, feel free.