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

[WIP] Upgraded Webpack to v4 #471

Merged
merged 15 commits into from
Mar 9, 2018
Merged

[WIP] Upgraded Webpack to v4 #471

merged 15 commits into from
Mar 9, 2018

Conversation

resir014
Copy link
Member

@resir014 resir014 commented Feb 11, 2018

Fixes #453

Very early work, but I'll continue working on it over the next week. Feedbacks appreciated.

## Also in this PR

* The current service worker method will not work with Webpack 4. I'm currently attempting to rewrite it using more modern Webpack plugins, like sw-precache-webpack-plugin. Currently looking at create-react-app's Service Worker setup and implementing portions of it here.
* Note that service worker will only load if the app is compiled under production mode (since sw-precache-webpack-plugin doesn't support WDS), so a new local server setup will probably have to be built. (see this link for example). This will be available on the npm tasks list as npm run start:prod.

Never mind. I've just been stupid all this time. SW's actually work.

WARNING: this still crashes. will look into it later.
This should now work. Note that benchmarks still necessary.

Also in this commit:
* Removed autoprefixer-loader in favour of postcss-loader chaining autoprefixer and postcss-flexbugs fixes
* Updated browserslist config for autoprefixer
@resir014
Copy link
Member Author

resir014 commented Feb 12, 2018

@dtinth The scripts should now work well with webpack 4! Note that this is still a very first attempt of getting it to work, more improvements could be done I think. (note how I haven't converted everything to using javascript/esm mode)

Also, since the generated bundles now look different, it'd be a good idea to run a quick benchmark comparing master with this branch.

Also for future reference: https://developers.google.com/web/fundamentals/performance/webpack/decrease-frontend-size

@resir014 resir014 requested a review from dtinth February 12, 2018 08:38
Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Wow, thanks, this looks really good!

Thanks for upgrading the webpack configuration and migrate several stuff to use the best practices (e.g. using module.rules instead of inline loader syntax, and replacing autoprefixer-loader).

Feel free to merge when you think it’s stable enough! :D

@resir014
Copy link
Member Author

resir014 commented Feb 14, 2018

@dtinth Yeah, we're almost done. Although two new test cases failures due to the change in how we load worker-loader and I haven't been able to figure out how to fix it. ;-;

image

Also I have yet to figure out how javascript/esm mode works, so I guess for now we'll stick to the default. I'll do some more polishing when I get back home tonight, and we can merge.

@resir014
Copy link
Member Author

Update: Service workers are also broken with this upgrade, will rewrite + bring it up to date w/ the current best practices of including a service worker. Currently looking at create-react-app's templates to look at how they do it.

@resir014
Copy link
Member Author

resir014 commented Feb 16, 2018

@dtinth The current service worker method will not work with Webpack 4. I'm currently attempting to rewrite it using more modern Webpack plugins. Currently looking at create-react-app's Service Worker setup and implementing portions of it here.

Note that service worker will only load if the app is compiled under production mode (since sw-precache-webpack-plugin doesn't support WDS), so a new local server setup will probably have to be built. (see this link for example). This will be available on the npm tasks list as npm run start:prod.

I will need another review from you once this is done.

@resir014
Copy link
Member Author

resir014 commented Feb 16, 2018

Actually, you know what, I should've split the SW rewrite to a separate PR /: Doing so now and hard-resetting this PR back to f22d83b, the latest stable commit.

Never mind. I've just been stupid all this time. SW's actually work.

@resir014
Copy link
Member Author

resir014 commented Mar 8, 2018

New errors:

image

image

These errors seem to appear from modules that need to be required via val-loader. Some of these may be fixable by moving them to webpack.DefinePlugin.

@dtinth
Copy link
Member

dtinth commented Mar 8, 2018

@resir014 The interface of val-loader have changed... Before that we can export a JavaScript string, e.g.

module.exports = 'module.exports = 42'

Now, it must look like this:

module.exports = () => ({ code: 'module.exports = 42' })

I think it is fine to keep using val-loader — we are simply compiling some code in compile time.

@dtinth
Copy link
Member

dtinth commented Mar 8, 2018

@resir014 I just pushed a commit to make it build successfully for now 😄.

@resir014
Copy link
Member Author

resir014 commented Mar 8, 2018

@dtinth Thanks! :D

As soon as we can figure out what causes the promise rejection failures in the tests, this should be ready to merge. Unfortunately, I've been trying to track down the issue for weeks but nothing came up 😓

@dtinth
Copy link
Member

dtinth commented Mar 9, 2018

@resir014 No worries, I will take care of it. Strangely enough, when running tests directly (http://localhost:8080/?mode=test) they all pass, but when run in Karma it does not work.

This may have to do with karma-webpack and its interaction with the web server (this may be related but not sure: codymikol/karma-webpack#295).

@dtinth
Copy link
Member

dtinth commented Mar 9, 2018

I am fixing this.

First, I started karma manually using

BROWSER=Chrome ./node_modules/.bin/karma start

Next, I clicked the “Debug” button and viewed the console.

image

This seems to be an issue with assets path again…

@dtinth
Copy link
Member

dtinth commented Mar 9, 2018

I did a weird workaround about karma-webpack (problem shown in the linked thread), somehow, in this branch it did not work anymore. However, the bug in karma-webpack has been merged and released in karma-webpack@2.0.10. This should now work :D

@dtinth
Copy link
Member

dtinth commented Mar 9, 2018

Ah, I remember now! I specified name=song-loader/ so that the worker file end up inside a directory. This works around the linked bug in karma-webpack.

import Worker from 'worker-loader?name=song-loader/[hash].worker.js!./worker.js'

The loader reference has been moved into module.rules, but the name option has not been specified, the worker file ended up inside the publicPath (not in a subdirectory) which breaks karma-webpack.

I hope this clears up the confusion, and sorry for causing you the trouble of tracking down a very subtle bug.

This should be mergeable now, yay 😄

@resir014
Copy link
Member Author

resir014 commented Mar 9, 2018

@dtinth Ah I see now! Yep, all green now. I'm merging this.

Thanks for your help! ❤️

@resir014 resir014 merged commit 83d6e0a into master Mar 9, 2018
@resir014 resir014 deleted the 453-webpack-v4 branch October 3, 2018 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants