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

upgrade to webpack3 and add scope hoisting #150

Closed
wants to merge 17 commits into from

Conversation

hassanbazzi
Copy link
Member

No description provided.

@hassanbazzi
Copy link
Member Author

Not quite sure why this is failing. I have a feeling it has to do with dependency weirdness. Will fix.

@developit
Copy link
Member

developit commented Jun 29, 2017

@hassanbazzi webpack in this context is import { webpack } from '@webpack-blocks/webpack2' - we might need to import the real one instead or just do an inline require for now.

@hassanbazzi
Copy link
Member Author

Ahhh yesss

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

I like this 👍

However, I don't think it's ready for merging yet simply because @webpack-blocks/webpack2 still installs webpack@2.x.

We'll be installing 2 versions of webpack until we either:

  1. move away from webpack-blocks entirely; or
  2. wait for them to upgrade/offer a webpack3 version

I'd assume that (2) is happening sooner rather than later...

Looks great tho @hassanbazzi, thanks!

Copy link
Collaborator

@rkostrzewski rkostrzewski left a comment

Choose a reason for hiding this comment

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

This line should be using webpack@3 https://github.com/developit/preact-cli/blob/master/src/lib/webpack/transform-config.js#L4
otherwise folks with preact.config.js will get old webpack

@developit
Copy link
Member

I'd be up for moving off of webpack-blocks. We can make it really really easy for ourselves too, all we need is a compose() function (I believe there are even webpack-specific ones) that lets us combine configurations the way we use blocks right now. Then just swap the custom blocks in for partial configs.

@lukeed
Copy link
Member

lukeed commented Jul 1, 2017

@developit Would webpack-merge work?

@developit
Copy link
Member

developit commented Jul 1, 2017

@lukeed yup, that was the one I was trying to remember the name of. I believe it just takes a bunch of webpack config partials and produces their combined result. We'd basically just be emulating customConfig() and addPlugins() since both are fairly useful.

Interestingly, these are already sortof mirrored in @rkostrzewski's helper lib - maybe we merge the two and use our own plugin/helpers API to construct the initial config? Just spitballing :)

@hassanbazzi
Copy link
Member Author

Sorry I'll put more work into this as soon as I'm back from vacation. <3

@lukeed
Copy link
Member

lukeed commented Jul 6, 2017

My bad 😬 Webpack release notes for V3 said that the UglifyJS plugin moved to a separate repo. In that repo, it had separate install instructions. It's still included in the main webpack package.

@kurtextrem
Copy link

Any news? Is there any help needed? :)

@rkostrzewski
Copy link
Collaborator

@developit @hassanbazzi I've done the work with removing webpack-blocks in favour of webpack-merge. Do we want to have our own blocks (right now it looks like a regular webpack config)?

@developit
Copy link
Member

I think your webpack-merge stuff looks good, not sure we need more than what's here. Were you just thinking it could be abstracted further? I'm five with either, this already looks cleaner than blocks (and more understandable) 👍

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Looks good! Exactly how I would have done it :D

@rkostrzewski
Copy link
Collaborator

@developit nah, I just saw in your previous comment some talk about abstracting stuff using things like addPlugin. IMHO not using such abstractions actually makes things simpler.

@hassanbazzi
Copy link
Member Author

This is awesome. Thanks for picking up @rkostrzewski

Just wondering, this was supposed to help us shed a bunch of bytes in polyfill.js. How come that changed?

@hassanbazzi
Copy link
Member Author

Ahhh. The webpack.optimize.ModuleConcatenationPlugin() was removed. Any reason why?

@lukeed
Copy link
Member

lukeed commented Jul 24, 2017

@hassanbazzi @rkostrzewski Happened in the merge from master. It was there yesterday when I approved.

package.json Outdated
"url-loader": "^0.5.8",
"webpack": "^2.3.3",
"url-loader": "^0.5.9",
"webpack": "^3.3.0",
Copy link

Choose a reason for hiding this comment

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

I'd definitely recommend using something higher than 3.3.0 if you're going to use ModuleConcatenationPlugin. I had to back out the use of the plugin in an internal app using 3.3.0 due to naming collisions and other strange demons. Check out the release notes of every release since 3.3.0 and you'll see module concat bugfixes. 👍 https://github.com/webpack/webpack/releases

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bwhitty Don't worry about that, semver will take care of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants