Skip to content

Hello Webpack4 ⭐️ #324

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

Merged
merged 79 commits into from
Jul 18, 2018
Merged

Hello Webpack4 ⭐️ #324

merged 79 commits into from
Jul 18, 2018

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented May 16, 2018

Hi guys!

I've delayed long enough. Let's upgrade to Webpack 4. As discussed on #250, we will not support old versions of Webpack - the versions are too different, and most people won't notice/care anyways.

And actually, other than the first todo, this is almost finished. AND, if you ignore yarn.lock, this PR makes Encore smaller :).

TODOS:

  • Fix createdSharedEntry(). I think we should just keep the existing functionality initially, and explore other options in the future. See Upgrade to Webpack 4 #250 (comment)

  • Test createdSharedEntry() with CSS

  • Verify that default cache groups still exist (are not completely overwritten) and warning if you create a shared entry called vendor.

  • Investigate the Webpack 4 output, and see if we can remove any of our special error handling or display supression.

  • remove vue-unactivated-loader

  • We should now support browserlist in package.json. Need to check into this (add a test) and update documentation as needed.

  • Check into fork-ts-checker-webpack-plugin@0.4.1" has unmet peer dependency "tslint@^4.0.0

  • update BIG jsdoc notes in versioning.js related to NamedModulesPlugin

  • Make sure CSS is being minified

  • Look into exposing the entry point chunks in a new JSON file

  • Can CSS chunks ever be split with cacheGroups?

  • Make the import() syntax work out-of-the-box

  • Add some "version checking" logic to, at least, our vue-loader code. This new version of Encore will only work with vue-loader 0.15.0 and higher. But, because this is an optional dep, nothing enforces that and the user will get strange errors if they use a lower version. There is a precedent set to check versions of optional deps at runtime.

  • When a CSS file references a non-existent image, the error is way off:

 ERROR  Failed to compile with 1 errors                                         8:32:32 PM

 error  in ./css/another_bg_image.css

Module build failed: ModuleNotFoundError: Module not found: Error: Can't resolve './../images/symfony_logo.png2' in '/Users/weaverryan/Sites/os/webpack-encore/tmp_project_playing/css'
(stacktrace)

A) I thought we cleaned up the stacktrace
B) the ModuleNotFound is listed several times
C) The path just ends in /css, not the filename, which is not that helpful.
D) The actual error is hidden in the ugly stack trace, not on top.

  • Create a solution to use the entrypoints.json file & deprecate createdSharedEntry()

Documentation

  • Document use of browserslist + update Encore recipe

Later

  • (Later, in another PR) change from webpack-dev-server to webpack-serve

@weaverryan weaverryan mentioned this pull request May 16, 2018
@@ -314,6 +306,24 @@ class ConfigGenerator {
.map((plugin) => plugin.plugin);
}

buildOptimizationConfig() {
const optimization = {

Copy link
Member

Choose a reason for hiding this comment

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

this needs to have runtimeChunk: 'single' to extract the runtime (and the list of BC breaks should mention the renaming from manifest.js to runtime.js)

@weaverryan
Copy link
Member Author

Just re-added the traditional createSharedEntry() functionality. As you hinted Stof, it was fairly straightforward - see sha: 183b146

About the default groups - vendor and default. First, I'm not sure if I've overwritten them, or if they still exist. Either way, they don't appear to be a problem, because they are only async. I need to look further, however, because I don't actually want to override them - they should still be available for async chunks.

@stof
Copy link
Member

stof commented May 18, 2018

yeah, as you enable splitting of the initial chunks only for your own cache group and not globally for all of them (as I was doing in my example in #250), they are totally fine (splitting the async chunks as much as webpack wants is totally fine)

@@ -308,6 +309,14 @@ class WebpackConfig {
this.babelConfigurationCallback = callback;
}

configureSplitChunks(callback) {
if (typeof callback !== 'function') {
throw new Error('Argument 1 to configureBabel() must be a callback function.');

Choose a reason for hiding this comment

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

throw new Error('Argument 1 to configureSplitChunks() must be a callback function.');

@weaverryan
Copy link
Member Author

weaverryan commented Jun 11, 2018

Ok! This is ready for review!

The file diff is huge, so I fear it's near impossible to review fully. However, the CHANGELOG has the highlights, including the BC breaks.

Anyone reading this, if you could try the webpack4 branch of this repo in your project, and report any issues, that would be a huge help :). You can do that by updating your package.json to have:

"@symfony/webpack-encore": "git://github.com/symfony/webpack-encore.git#webpack4"

And running yarn upgrade.

Fun fact: if you compare the functional tests of this branch to the master branch, they're noticeably faster (thanks to Webpack 4 being faster) :)

@raupie
Copy link

raupie commented Jun 12, 2018

I was having issues with running 'production' for a foundation-sites 6 install with the latest stable version of webpack-encore. I added this branch and now it's passing successfully! This just made my day, seriously. Win! Love webpack-encore!

@weaverryan
Copy link
Member Author

@raupie Woohoo! Thanks for testing this!

I have discovered one issue. If you use createdSharedEntry(), it appears that the code in your shared entry may not be executed :/. Checking into it...

@stof
Copy link
Member

stof commented Jun 13, 2018

@weaverryan how does the entrypoints.json file works regarding versionning ? does it contain versionned names, or non-versionned ones (to lookup in the manifest.json file) ?

CHANGELOG.md Outdated
* Introduced a new `configureSplitChunks()` method that can be
used to further configure the `optimizations.splitChunks` configuration.

* A new `entrypoints.js` file is now always output. For expert
Copy link
Member

Choose a reason for hiding this comment

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

is it a .js or a .json file ? I don't think it should be a JS file if the backend code should deal with it (I don't want to need to use a node-subprocess to read the JS file to allow for JS features)

Copy link
Member

Choose a reason for hiding this comment

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

the code is already using json so this is only a doc mistake


splitChunks.cacheGroups = cacheGroups;
// causes a runtime.js to be emitted with the Webpack runtime
optimization.runtimeChunk = 'single';
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be done by checking whether optimization.splitChunks is a non-empty object instead ? Now that you added support for a callback, the case of using sharedCommonsEntryName is not the only case triggering splitting.

'h1.8ec31654.css',
'bg.0ec2735b.css',
'manifest.json',
'entrypoints.json'
Copy link
Member

Choose a reason for hiding this comment

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

I suggest asserting the content of entrypoints.json too in this test using versionning

testSetup.runWebpack(config, (webpackAssert) => {
webpackAssert.assertOutputJsonFileMatches('entrypoints.json', {
main: {
js: ['vendors~main~other.js', 'main~other.js', 'main.js'],
Copy link
Member

Choose a reason for hiding this comment

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

should we have the runtime file in this list too ?

DeleteUnusedEntriesJSPlugin: 0,
EntryFilesManifestPlugin: 0,
Copy link
Member

Choose a reason for hiding this comment

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

should have a low priority, to be sure it runs after other plugins (btw, we should really change these priorities. Having them all at 0 makes the priority system a simple prepend/append one, which could then be simplified a lot)

lib/features.js Outdated
packages: [
{ name: 'typescript' },
{ name: 'ts-loader', version: 4 },
{ name: 'fork-ts-checker-webpack-plugin', version: 0 },
Copy link
Member

Choose a reason for hiding this comment

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

this will ask to install fork-ts-checker-webpack-plugin@^0.0, which won't work.

You should not handle version as a number IMO, but as a semver version constraint instead IMO (use the semver package to check that then).

plugin: new EntryFilesManifestPlugin(
path.join(webpackConfig.outputPath, 'entrypoints.json')
),
priority: PluginPriorities.DeleteUnusedEntriesJSPlugin
Copy link
Member

Choose a reason for hiding this comment

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

wrong priority constant (OK, they're all using the same value, so it is hard to notice it)

* @param {function} extractTextPluginOptionsCallback
* @returns {Encore}
*/
configureExtractTextPlugin(extractTextPluginOptionsCallback = () => {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could also do something in the EncoreProxy at the bottom of that file to detect when a user tries to call configureExtractTextPlugin or enableCoffeeScriptLoader and display some kind of "This method is not available anymore in Webpack Encore" error message instead of the default one?

@@ -115,7 +118,7 @@ class ConfigGenerator {
}

buildOutputConfig() {
// Default filename can be overriden using Encore.configureFilenames({ js: '...' })
// Default filename can be overridden using Encore.configureFilenames({ js: '...' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is the same typo twice below (my bad!)

@weaverryan
Copy link
Member Author

@stof

@weaverryan how does the entrypoints.json file works regarding versionning ? does it contain versionned names, or non-versionned ones (to lookup in the manifest.json file) ?

Actually, this is tricky. I believe it is currently the versioned file. But, I think that's a mistake: it should be the unversioned filename because we need to be able to loop up the value manifest.json (e.g. in case a CDN is used). In fact, I think the filenames in main.js need to be the same as the "keys" in manifest.json.

// entrypoints.json

{
    "main": {
        "js": ["build/runtime.js", "build/shared.js", "build/main.js"],
        "css": ["build/shared.css"]
    }
}

// manifest.json

{
    "build/runtime.js": "https://somecdn.com/build/runtime.abc123.js",
    "...": "..."
}

This would allow some future code like this in a template:

{% for jsFile in getEntrypointJavaScriptFiles('main') %}
        <script src="{{ asset(jsFile) }}"></script>
{% endfor %}

But I'm not sure yet if this entirely makes sense. Another option would create a simpler map:

// entrypoints.json

{
    "main": {
        "js": ["runtime.js", "shared.js", "main.js"],
        "css": ["shared.css"]
    }
}

Then, do a bit more work in the template:

{% for jsFile in getEntrypointJavaScriptFiles('main') %}
        <script src="{{ asset('build/'~jsFile) }}"></script>
{% endfor %}

(or some shortcut version)

{{ webpackEntryScriptTags('main', 'build/') }}

(or even put the build/ prefix in the config of that future bundle.

Thinking out loud - not sure about this exact detail yet.

@raupie
Copy link

raupie commented Jul 12, 2018

any update?

@weaverryan
Copy link
Member Author

Was just working on it today :). All major parts are done - just finishing touches. I’ll have it soon!

We need to re-add commons chunk still, but in a different way
Previously, the CSS was included inline in the JS file, and added via
the style loader. This should be invisible to the user.
In theory, they should not have. But, we upgraded so many libraries,
this is not very surprising. It should be invisible to the user anyways.
splitChunks.name = function(module, chunks, cacheGroup) {
const names = chunks.map(c => c.name);

return cacheGroup + '~' + hashFilename(names.join('~'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure ~ is a good separator choice, it's part of the "unsafe characters" listed in this RFC: http://www.ietf.org/rfc/rfc1738.txt

Copy link
Member

Choose a reason for hiding this comment

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

that's the separator used by default in Webpack 4

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kinda weird they went with that one, but let's keep it that way then

This is a subjective decision we made, because we thought it would cause
less "confusing" moments (i.e. when 2 different entries would not share
the same modules).

Unfortunately, the single runtime chunk causes a strange bug with
mini-css-extract plugin where, under some conditions, the output CSS
uses the chunk filename instead of "filename".
This removes the BC breaks related to runtime.js, in favor of a
deprecation layer instead.
This will make it SUPER easy to find the final paths:
1) Look up all the CSS or JS files for entry "foo_bar"
2) The values you get back can immediately be used to lookup the
    final path in manifest.json
@weaverryan weaverryan merged commit c247131 into master Jul 18, 2018
@weaverryan weaverryan deleted the webpack4 branch July 18, 2018 19:30
@weaverryan
Copy link
Member Author

Woohoo!

I've just merged this! But, I'm not ready to create a tag: I need to make a PR for the documentation and discuss the plan for the WebpackEncoreBundle. But, it should now be a bit easier to test Webpack4 - just use the master branch - update your package.json file:

"@symfony/webpack-encore": "git://github.com/symfony/webpack-encore.git#master"

@arendjantetteroo
Copy link

Nice work!

I've tried updating and got 2 errors

  1. style-loader was missing, not sure if this was a previous requirement? (run yarn add style-loader fixed it)
  2. An error about a plugin/preset doing some things not allowed:

$ yarn run encore dev --watch
yarn run v1.7.0
warning package.json: No license field
$ /node_modules/.bin/encore dev --watch
/node_modules/@babel/core/lib/config/config-descriptors.js:138
throw new Error(Plugin/Preset files are not allowed to export objects, only functions. In ${filepath});
^

Error: Plugin/Preset files are not allowed to export objects, only functions. In /node_modules/babel-preset-react/lib/index.js

We use enableReactPreset.
Am i missing a change to make?

@stof
Copy link
Member

stof commented Jul 19, 2018

maybe you need to update the preset to a newer version

@arendjantetteroo
Copy link

Thanks, that was indeed it.

@raupie
Copy link

raupie commented Jul 19, 2018

I'm getting this error: SyntaxError: unexpected token: ':' which points to line one below.

/***/ "../../../tmp/tmp-92SW7NIosJEwEF.tmp":
/*!***********************************!*\
  !*** /tmp/tmp-92SW7NIosJEwEF.tmp ***!
  \***********************************/
/*! no static exports found */
/***/ (function(module, exports, __webpack_require__) {

@weaverryan
Copy link
Member Author

weaverryan commented Jul 23, 2018

@arendjantetteroo What version of react-preset were you using before? And did you see any warning that you needed to upgrade? After this PR, if you're using an optional library that is outside of our supported version constraints, you should see a warning. I'm worried there may be some issue with that - would love some more info from you on #359. Thanks!

style-loader was missing, not sure if this was a previous requirement? (run yarn add style-loader fixed it)

This is mysterious, because it IS a under dependencies, so it should have been installed automatically. Basically, this should work without needing to add it. @arendjantetteroo if you yarn remove style-loader do things still work?

@raupie Can you post your setup on #360? You are using createSharedEntry(), right? It indeed looks like a possible bug - but I'll need help reproducing. This is now fixed

@arendjantetteroo
Copy link

@weaverryan Sorry, holiday came in between.

We had "babel-preset-react": "^6.24.1", in yarn.
No style-loader in package.json

Upgraded to "@babel/preset-react": "^7.0.0-beta.54" fixed the error (but caused a lot of other dependency to be needed to upgrade as well)

Removing the style-loader it now seems to work fine, might have been an issue with all the dependency updates or leftover webpack config.

@Rebolon
Copy link

Rebolon commented Oct 10, 2018

Hi,
any documentation on how to use this great feature ?
what is the standard configuration to use ?
Thx

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.

9 participants