Skip to content
This repository was archived by the owner on Sep 28, 2020. It is now read-only.

fix(loader.options): use this.query to retrieve options #210

Closed
wants to merge 1 commit into from
Closed

fix(loader.options): use this.query to retrieve options #210

wants to merge 1 commit into from

Conversation

hitmands
Copy link

@hitmands hitmands commented Feb 11, 2018

@jsf-clabot
Copy link

jsf-clabot commented Feb 11, 2018

CLA assistant check
All committers have signed the CLA.

@MoOx
Copy link
Contributor

MoOx commented Feb 12, 2018

Could you try to fix tests?

@hitmands
Copy link
Author

I did fix them, can we merge please?

@MoOx
Copy link
Contributor

MoOx commented Feb 13, 2018

Sorry but this will require more work before this end up being merged.

Very sorry to say that but this PR is clearly far from what is required to support webpack 4, properly. You need to add webpack 4 at least in travis.yml and in package.json in dev deps.

This will pop out a lot of error related to the current way this loader has been tested among various versions of webpack.

I guess the easiest way is to decide to make a major bump in the version (2.x) and drop all other webpack version (this way, no mess with webpack 1, 2, node 4... and previous way of loading options (and most importantly the tests related to this).

@TheLarkInn
Copy link

TheLarkInn commented Feb 16, 2018

@MoOx were you interested in forking into a "next" branch. This is typically the way we have handled the v4 migrations and blasting it off as a major version.

@MoOx
Copy link
Contributor

MoOx commented Feb 16, 2018

I would more go to keep a stable branch and work on master. Anyway, I think a PR can do the trick. The work need to be done at once imo. Even if this single line seems to be enough (it don't in practise), we need to run tests on the proper version (and break retro compat if necessary + add a version matrix at the beginning of the readmy)

@hitmands
Copy link
Author

I submitted this pr to provide a basic support for webpack 4.

@MoOx
Copy link
Contributor

MoOx commented Feb 16, 2018

Yes, thanks for this.
But I tried locally to use webpack 4 and tests are completely broken. So we cannot ship this as it is imo.

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

Successfully merging this pull request may close these issues.

4 participants