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

Make webpack CommonsChunkPlugin chunking default configurable #3600

Closed

Conversation

gcpantazis
Copy link
Contributor

A while ago (in #1644) we updated the default for minimum refs to chunk a file into commons from the previous (which I believe was a static integer, or at least was at some point), and made it so that the file had to be used in >50% of the pages to be packaged in the commons.

The problem is that people may use pages in different ways, particularly if they're using a custom server instead of the default page routing. For example, I use Styled Components, and put a styles.js file alongside my various page JSX files. Others may colocate JSX components inside pages, which could be very reasonable depending on the project.

In any case, it's very possible to load up your /pages directory with JS files that add into the default logic's perception of "page count", since there's no singular way to determine what is and is not a page without analyzing the code within.

I'd propose that leave this as the default behavior, since that makes sense enough if you're using Next in the OOTB manner, but allow for configuration if needed. @rauchg suggested as much in #1644 but it seems like that fell out in the final version (#1659)

If this seems good to all I'll add examples / update docs. LMK, thanks!

A while ago (in vercel#1644) @arunoda updated the default for minimum refs to
chunk a file into commons from the previous (which I believe was a
static integer, or at least was at some point), and made it so that the
file had to be used in > 50% of the pages.

The problem is that people may use pages in different ways, particularly
if they're using a custom server instead of the default page routing.
For example, I use Styled Components, and put a `styles.js` file
alongside my various page JSX files. Others may colocate JSX components
inside pages, which could be very reasonable depending on the project.

The point is, it's very possible to load up your /pages directory with
JS files that add into the default logic's perception of "page count",
since there's no singular way to determine what is and is not a page
without analyzing the code within.

I'd propose that leave this as the default behavior, since that makes
sense if you're using Next in the OOTB manner, but allow for
configuration if needed. @rauchg suggested as much in vercel#1644 but it seems
like that fell out in the final version (vercel#1659)
@@ -5,6 +5,15 @@ const cache = new Map()
const defaultConfig = {
webpack: null,
webpackDevMiddleware: null,
webpackModuleShouldBeCommonInProduction: function ({totalPages, count}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pretty long function name and it's not so clear, I understood by the name that I'll be able to define if an individual module should be in the common chunks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, fair enough. Technically you could/(should) pass the whole module into this method if it were to exist, which would make the method name correct (if long).

In any case I think I'll abort this approach per @timneutkens's comment

@timneutkens
Copy link
Member

timneutkens commented Jan 22, 2018

In any case, it's very possible to load up your /pages directory with JS files that add into the default logic's perception of "page count", since there's no singular way to determine what is and is not a page without analyzing the code within.

When you start adding components and styles inside pages you'll run into much bigger problems than the commonchunk calculation. Every file inside pages is a webpack entrypoint, meaning that if you put all your components in pages it'll introduce a heavy cpu, memory and time weight on the whole build.

Considering this, the rule is pretty simple: every file inside of pages is a page, and shouldn't be something else 😄
I guess we can do a better job of explaining that in the documentation.
In a future release we might provide a way to define which pages are actually pages and ignore the rest.

Regarding the added functionality, this can be added using webpack in next.config.js. The only thing missing for that is totalPages but we could add that in the object passed as second argument.

@gcpantazis
Copy link
Contributor Author

All good points @timneutkens.

Considering this, the rule is pretty simple: every file inside of pages is a page, and shouldn't be something else 😄

Hah. I think this is a good basic rule, sure. I have my one use-case (separate styles.js files) and I don't have enough others to say it shouldn't be the rule 😄

Just so I'm gauging the philosophy correctly: the project wants to discourage top-level configuration in config.js except in rare cases where it has to be there to support common use-cases. For "advanced" overrides that are deeper edge-cases (like mine), we're okay with supporting a fix, but it should be done by customizing webpack.

Or put another way, if there's a clear way to do it with webpack, we should be doing it with webpack, not config.

Regarding the added functionality, this can be added using webpack in next.config.js. The only thing missing for that is totalPages but we could add that in the object passed as second argument.

Yeah this makes sense. I'll see if there's anything that needs to be changed (passing totalPages etc.) to get this working.

@kochis
Copy link

kochis commented Feb 1, 2018

We have a use case that supports this feature as well. Within our app, we have an internal facing portal that is name-spaced within pages /pages/internal/.... Ideally, we could exclude those pages from commons since it's not part of the critical path (a lot of libs used there aren't needed in the rest of the app).

@timneutkens
Copy link
Member

You can do it like this: https://github.com/zeit/next-plugins/blob/master/packages/next-css/commons-chunk-config.js

@timneutkens
Copy link
Member

Closing this in favor of #3824

@lock lock bot locked as resolved and limited conversation to collaborators Feb 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants