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

Migrate to webpack 4 #93

Merged
merged 17 commits into from Feb 25, 2018

Conversation

Projects
None yet
4 participants
@johnnyreilly
Copy link
Collaborator

johnnyreilly commented Jan 27, 2018

Resolves #78 and has been tested with ts-loader@4.0.0-beta.0

So I've had a busy day porting the plugin to webpack 4. I think we're pretty much there now. Yay!

All the tests are passing apart from 2 Vue integration tests:

  1. should not find syntactic errors when checkSyntacticErrors is false
  2. should find syntactic errors when checkSyntacticErrors is true

The reason these are failing is that there are extra errors surfacing when these tests are run:

TypeError: Cannot read property 'vue' of undefined

My feeling is that this is probably because vue-loader has yet to be ported to webpack 4. On that basis I'd ignore these failing tests for now.

If there's any Vue peeps out there that could advise on vue-loader supporting webpack 4 that'd be awesome.

cc @piotr-oles

UPDATE

It looks like the vue-loader needs work to be webpack 4 compatible. See here: vuejs/vue-loader#1142

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Jan 27, 2018

Since I can't publish fork-ts-checker-webpack-plugin I'll probably try and publish this on GitHub sometime soon so people can have a play ahead of time.

PS @piotr-oles if you could requeue Travis that'd be helpful; it had a moment and didn't actually start running the tests. Thanks!

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Jan 27, 2018

Don't worry - I worked out what the original test run issue was; insufficient node version. It now runs against node v8 in Travis. I also added myself as a contributor in the package.json - happy to remove if that's an issue.

@piotr-oles

This comment has been minimized.

Copy link
Member

piotr-oles commented Jan 31, 2018

Please resolve conflicts and if tests will pass, we will be ready to merge :) Thank you for your work ;)

},
"peerDependencies": {
"tslint": "^5.0.0",
"typescript": "^2.1.0",
"webpack": "^2.3.0 || ^3.0.0"

This comment has been minimized.

@piotr-oles

piotr-oles Jan 31, 2018

Member

|| ^4.0.0? :)

This comment has been minimized.

@piotr-oles

piotr-oles Jan 31, 2018

Member

It will be backward compatible, right?

This comment has been minimized.

@johnnyreilly

johnnyreilly Jan 31, 2018

Author Collaborator

Perhaps - I'm not sure to be honest. Happy to add ^4.0.0 though.

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Jan 31, 2018

Thanks for reviewing @piotr-oles! - As soon as I get a moment I'll resolve the conflicts and report back.

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 4, 2018

Hey @piotr-oles,

I've merged in the changes.

Here's where we are:

  • non Vue tests pass as they did before
  • Vue tests pass! I've migrated the plugin to use version 14.1.0 of vue-loader - this should support webpack 4 according to @yyx990803 here

It's not clear how backwards compatible the webpack 4 changes are. To quote @sokra here: "The plugin method on tapable objects is theoretically backward-compatible"

I've bumped to version number of the package to 0.4.0 as that seems reasonable. What do you think?

cc @prograhammer @CKGrafico @wonderful-panda @HerringtonDarkholme

I've pushed out a new beta here: https://github.com/johnnyreilly/fork-ts-checker-webpack-plugin/tree/4.0.0-beta.1

  • when using yarn: yarn add johnnyreilly/fork-ts-checker-webpack-plugin#4.0.0-beta.1 -D
  • when using npm: npm install johnnyreilly/fork-ts-checker-webpack-plugin#4.0.0-beta.1 -D

More details here: https://blog.johnnyreilly.com/2018/01/webpack-4-ts-loader-fork-ts-checker.html

@piotr-oles

This comment has been minimized.

Copy link
Member

piotr-oles commented Feb 4, 2018

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 4, 2018

I've added in a script that runs integration tests against webpack 2, 3 and 4. As you can see, the tests fail with webpack 2 and 3. I don't think we can rely on the changes required for webpack 4 being backwards compatible.

That being the case, how do you want to proceed? In case it helps, here's what I'm planning for ts-loader:

  • When webpack 4 is released I'm going to create a webpack-3 branch of ts-loader.
  • I'm then going to release ts-loader 4.0 which will support webpack 4 as a minimum.
  • Those people that need webpack 2/3 can continue using ts-loader 3.x. I don't plan any further ts-loader 3.x releases but if there's a real need I'll put a change in the 3.x branch and cut a release.

So what would you like me to do? Shall I remove webpack 2 and 3 from the peerDependencies and drop the integration tests against webpack 2 and 3?

Maintaining 2 versions of the plugin would be rubbish. But theoretically, further releases of the plugin for webpack 2/3 will hopefully not be necessary. As long as there's something in the README that says "If you need webpack 2/3 support then install v0.3.x of the plugin" I'd say that's probably good enough.

What do you think?

cc @sokra

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 5, 2018

I've been doing some digging and I discovered an approach suggested by @jeffposnick here: GoogleChrome/workbox#1151

This will make the code more complicated (tests too) but might allow us to achieve backwards compatibility. Could be worth trying out?

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 6, 2018

I think the approach in place works. The builds are failing on webpack 2 and 3 because config has to be slightly different for webpack 2 and 3 than it does for 4.

I think I need to amend the integration tests to check the version of webpack being used and only supply the mode: 'development' option when it is webpack 4.

Not sure the best way to achieve that but I'll have a think. Suggestions welcome!

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 7, 2018

Okay - this has not gone as well as I'd hoped. I'm now conditionally providing different webpack configs depending on the webpack version.

As expected, webpack 4 tests still all pass.

With webpack 2/3 there's 1 integration test failure (which may not be a true failure) in the standard (index.spec.js) test pack. This is what I want to look into further; it doesn't seem that there is any difference when I've been console.loging the error differences locally. Still, chai disagrees and I'll investigate further.

More problematically; the Vue tests are failing in the webpack 2 / 3 branch. I've tried rolling back to the older (13.5) version of vue-loader but that seems to make no difference. I don't know much about Vue and I'm not likely to get much further with that.

If anyone who does know something about Vue can help with that I'd appreciate it. cc @prograhammer @CKGrafico @wonderful-panda. Feel free to fork my repo and see if you can succeed where I've failed.

FWIW, I suspect the plugin works with webpack 2/3 just fine. However, persuading the integration tests to agree is where the trouble likely lies. I'm just not sure as to why.

If no-one's is able to help out with the Vue issues I'm likely to suggest we drop support for webpack 2 / 3 in the same codebase. The added complexity isn't getting us to a good place so far.

Thoughts?

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 7, 2018

Boom. webpack 2, 3 and 4 supported in this PR. I thank you.

@prograhammer

This comment has been minimized.

Copy link
Contributor

prograhammer commented Feb 7, 2018

Sorry, I've been slammed with work lately. But this looks exciting. I hope Webpack 4 will be LTS because Webpack is sort of changing a little too quickly IMO. Thanks @johnnyreilly for the awesome work!

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 7, 2018

@prograhammer no bother - got there in the end! Yeah I'll be super happy if the move from webpack 4->5 is like the webpack 2-3 migration (ie seemless)

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 7, 2018

Hey @piotr-oles

This can be merged and released whenever you like. 🛳️

johnnyreilly added some commits Feb 12, 2018

@fruchtose

This comment has been minimized.

Copy link

fruchtose commented Feb 13, 2018

I would love to see this merged into a new release!

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 13, 2018

Hey @fruchtose,

Until it is merged and released, you can try the beta. Details here: https://blog.johnnyreilly.com/2018/01/webpack-4-ts-loader-fork-ts-checker.html

johnnyreilly added a commit to TypeStrong/fork-ts-checker-webpack-plugin that referenced this pull request Feb 20, 2018

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 20, 2018

I've published this to https://github.com/typestrong/fork-ts-checker-webpack-plugin (what we're using as an @next channel for fork-ts-checker-webpack-plugin)

See the readme.md for details on how to use it in your package.json

johnnyreilly added some commits Feb 23, 2018

@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 25, 2018

webpack 4 has now shipped! 🍾

I've released ts-loader 4.0 and I've published this to https://github.com/typestrong/fork-ts-checker-webpack-plugin - early adopters can use this until @piotr-oles has chance to merge and publish.

https://github.com/TypeStrong/fork-ts-checker-webpack-plugin

See blog post for more details: https://blog.johnnyreilly.com/2018/02/ts-loader-400-fork-ts-checker-webpack.html

@piotr-oles

This comment has been minimized.

Copy link
Member

piotr-oles commented Feb 25, 2018

Thank you for your work, will be released as 4.0 :)

@piotr-oles piotr-oles merged commit 6d59404 into Realytics:master Feb 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johnnyreilly

This comment has been minimized.

Copy link
Collaborator Author

johnnyreilly commented Feb 25, 2018

Nice! Thanks @piotr-oles! I've updated the blog post to reference this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.