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

Trying out the new webpack-manifest-plugin #164

Merged
merged 2 commits into from
Apr 14, 2018
Merged

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Sep 21, 2017

This is not ready to be merged yet... as we should wait for a stable release. But, let's see if the tests pass :). We originally "forked" this library internally due to 2 issues, both of which should have been fixed in version 2 of the library.

Should fix #140.

TODOS:

  • Remove the old webpack-manifest-plugin.js file entirely and update paths to require the actual library instead of this

  • Stop passing the publicPath option to the plugin, as this is not supported anymore (I believe it's just being ignored right now)

@weaverryan
Copy link
Member Author

Failures are due to: salesforce/tough-cookie#92... which is unrelated to us, and will hopefully get resolved shortly.

@stof
Copy link
Member

stof commented Sep 21, 2017

yeah, too bad that JS regexp does not have possessive quantifiers. In PHP, they would allow fixing the issue very easily (we did such fixes in Symfony).

@Lyrkan
Copy link
Collaborator

Lyrkan commented Oct 6, 2017

Remove the old webpack-manifest-plugin.js file entirely and update paths to require the actual library instead of this

I'd keep the file for now with only the export of the real plugin and a logger.deprecation(...) in it in case someone currently uses it to do something like:

const Encore = require('@symfony/webpack-encore');
const ManifestPlugin = require('@symfony/webpack-encore/lib/webpack/webpack-manifest-plugin.js');

// (...)

const config = Encore.getWebpackConfig();

for (const plugin of config.plugins) {
    if (plugin instanceof ManifestPlugin) {
        // Do something with its config
    }
}

module.exports = config;

@weaverryan
Copy link
Member Author

Hmm, interesting point - I'm cool with that :). In general, however, we should discourage people from requiring specific files from the lib - these shouldn't be considered part of our public API, unless we advertise using them (like the new PluginPriorities module).

@Lyrkan
Copy link
Collaborator

Lyrkan commented Apr 11, 2018

@weaverryan It looks like Webpack Manifest Plugin 2.0.0 is out :)

@weaverryan
Copy link
Member Author

Yeeeeea! @Lyrkan I just fixed conflicts, let's wait for the tests. Can you review the PR one last time to give it a 👍 ? Then we can merge and finish copyFiles() :)

@@ -46,6 +46,7 @@
"webpack": ">=2.2.0 <4",
"webpack-chunk-hash": "^0.5.0",
"webpack-dev-server": "^2.4.5",
"webpack-manifest-plugin": "v2.0.0-rc.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be "webpack-manifest-plugin": "^2.0.0" ?

@weaverryan weaverryan merged commit 6657a78 into master Apr 14, 2018
weaverryan added a commit that referenced this pull request Apr 14, 2018
This PR was squashed before being merged into the master branch (closes #164).

Discussion
----------

Trying out the new webpack-manifest-plugin

This is not ready to be merged yet... as we should wait for a stable release. But, let's see if the tests pass :). We originally "forked" this library internally due to 2 issues, both of which should have been fixed in version 2 of the library.

Should fix #140.

TODOS:

- Remove the old `webpack-manifest-plugin.js` file entirely and update paths to require the actual library instead of this

- Stop passing the `publicPath` option to the plugin, as this is not supported anymore (I believe it's just being ignored right now)

Commits
-------

6657a78 using the module directly and removing publicPath option, which is gone
9f7a866 Trying out the new webpack-manifest-plugin
@weaverryan weaverryan deleted the true-manifest-plugin branch April 14, 2018 18:51
@ghost ghost mentioned this pull request Apr 20, 2018
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.

How to manually add entries to manifest.json?
3 participants