Skip to content
This repository has been archived by the owner. It is now read-only.

Remove check for filenameTemplate #159

Merged

Conversation

@claus
Copy link
Contributor

@claus claus commented May 2, 2018

This broke commons-chunk-config in Next 6.

Helps to fix #157

This broke commons-chunk-config in Next 6.
@@ -3,16 +3,18 @@ module.exports = (config, test = /\.css$/) => {
config.plugins = config.plugins.map(plugin => {
if (
plugin.constructor.name === 'CommonsChunkPlugin' &&
(plugin.filenameTemplate === 'commons.js' || plugin.filenameTemplate === 'main.js')
typeof plugin.minChunks !== 'undefined'
Copy link
Member

@timneutkens timneutkens May 2, 2018

Choose a reason for hiding this comment

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

Lets keep the existing check, since otherwise this breaks 5.0/5.1

Loading

Copy link
Contributor Author

@claus claus May 2, 2018

Choose a reason for hiding this comment

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

Heh, but the existing check is what breaks 6

Loading

Copy link
Member

@timneutkens timneutkens May 2, 2018

Choose a reason for hiding this comment

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

We need to do both checks in an or

Loading

Copy link
Contributor Author

@claus claus May 2, 2018

Choose a reason for hiding this comment

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

Like this? plugin.constructor.name === 'CommonsChunkPlugin' && (plugin.filenameTemplate === 'commons.js' || plugin.filenameTemplate === 'main.js' || typeof plugin.minChunks !== 'undefined')

Loading

Copy link
Contributor Author

@claus claus May 2, 2018

Choose a reason for hiding this comment

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

Doesn't feel right :)

Loading

Copy link
Contributor Author

@claus claus May 2, 2018

Choose a reason for hiding this comment

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

Are you sure the removal breaks 5?

Loading

Copy link
Contributor Author

@claus claus May 2, 2018

Choose a reason for hiding this comment

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

I just tested it in Next 5.1 and it didn't seem to break it. Why exactly do you check for commons.js or main.js?

Loading

Copy link
Member

@timneutkens timneutkens left a comment

Actually this will work backwards compatible too.

Loading

@timneutkens timneutkens merged commit 084a985 into vercel:master May 2, 2018
@visualfanatic
Copy link

@visualfanatic visualfanatic commented May 2, 2018

Thank you @claus for the fix! 👍

Loading

@RobbinHabermehl
Copy link

@RobbinHabermehl RobbinHabermehl commented May 7, 2018

This PR resolves our issues as well, thanks @claus!

@timneutkens, could you please publish this to NPM and also bump the dependency in @zeit/next-sass? Thanks!

Loading

@timneutkens
Copy link
Member

@timneutkens timneutkens commented May 9, 2018

Released the new versions.

Loading

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

Successfully merging this pull request may close these issues.

4 participants