Nextjs/server modules included in the client bundle

Question

Nextjs/server modules included in the client bundle

michielmetcake opened this issue 2 years ago · 20 comments

We're running into compiling our code to es5 because the client code includes next/server modules. This increases the bundle size unnecessary.

Example here: https://github.com/michielmetcake/tss-react/tree/main/src/test/apps/ssr

Run yarn analyze and check the client bundle.

Answer 1 · 2023-02-15T14:33:42.000Z

Hello @michielmetcake,

I'm not sure that I understand, what's for sure is that next/server is never imported in tss-react.
I think I'm only importing what's need to be imported and nothing more.
If you can prove me I'm wrong by providing a clean Next.js project with only tss-react setup I'll swiftly address the issue.

My test app isn't great to analyze what's included, there is to much noise.
Beside, when I run yarn analyze I don't see any server

I might be missunderstanding what you're saying though...

Answer 2 · 2023-02-15T14:50:13.000Z

Thanks for your reply @garronej
If you zoom in a bit more. Then you'll see the next/server modules. It's inherited from the next/document

In your example I see pretty-bytes.js which is part of next/server

Answer 3 · 2023-02-15T15:27:06.000Z

Well indeed next/document is imported.

But I mean next/document has to be imported, there's no way around it.
Look it's imported in the reference Next.js setup for MUI: https://github.com/mui/material-ui/blob/0c6003420b0edab42dc0f411bbb51c16aac2697e/examples/nextjs-with-typescript/pages/_document.tsx

Am I missing somethin?

Answer 4 · 2023-02-15T15:30:12.000Z

The example provided is not having the document in the app. Because your augmentDocumentWithEmotionCache function is used in both _document and _app it's having the next/document imported in the _app. And this is causing issues with server scripts ending up in the client. I think the solution would be to split the functions into two files so the logic for the _app and _document are separate.

Answer 5 · 2023-02-15T16:10:56.000Z

Okay!
Now it makes perfect sense, thank you for sorting this out!

I'll see what I can do

Answer 6 · 2023-02-15T16:17:18.000Z

I'll make a proposal for you. I'm at 90%, will create a PR tomorrow :-)

Thanks for maintaining this!

Answer 7 · 2023-02-15T16:25:15.000Z

There could be a way without API changes unsing dynamic imports.

Thank you for the kind words, I'll review your PR tomorrow then!

Answer 8 · 2023-02-16T07:53:09.000Z

Created a solution here: #148

Answer 9 · 2023-02-20T09:27:00.000Z

I still see some next/server packages in the _app bundle. I don't think the dynamic import had the effect that I'm looking for. I think the best (maybe only way) of fixing it would be 2 different functions.

You can run yarn analyze in the nextjs/ssr app and you'll see next/server packages in there.

Answer 10 · 2023-02-20T10:17:48.000Z

I still see some next/server packages in the _app bundle.

Damn it :(
Do you agree that it works in this demo repo?
#149 (comment) (real question here, I want to be sure that we are on the same page and that I'm not missing something. If you could run analyze on this it would be great).

I think the best (maybe only way) of fixing it would be 2 different functions.

If there is no other way, I'll merge your PR but I think there is a way to make it work with dynamic imports.
If it worked in my test repo, I just need to figure out why it doesn’t in your case and address the issue.
Maybe in your case dynamic import doesn’t work because Next is using the CJS distribution instead of the ESM...
A step by step reproduction path would greatly help.

Answer 11 · 2023-02-20T10:55:58.000Z

Yes It's the same. I've a branche here based on the main branch of this repo.

I think you shouldn't pass data or imports from the _app to the _document. The other way around is allowed because you can enrich the app in the document.

Answer 12 · 2023-02-20T11:14:45.000Z

Yes allright I realized my mistake.

Thanks for your help. (See my answer on your PR)

Answer 13 · 2023-03-02T13:41:54.000Z

Fixed in 4.7 with dependency injection: https://docs.tss-react.dev/migration-guides/v4.6-greater-than-v4.7

Answer 14 · 2023-03-02T16:24:28.000Z

Thanks @garronej. I still see some issues with this release as a result of importing app code into the document. I see a few deps shipped in the _app bundle which I don't expect there. Like next/dist/compiled/stream-browserify, html-tokensize and readble-stream.

1 function to both serve the _Document and _App is a nice idea. But I think the best way is to split them into two so the code and deps for the _app stay in the _app and code for the _document stay in the _document.

Answer 15 · 2023-03-02T17:34:10.000Z

Damn it,
I was so sure it would do it that I didn't took the time to test.
My bad, I'm back at it.

Answer 16 · 2023-03-02T21:02:39.000Z

Hi,
I solved it for good this time, actually tested the thing.
I figured out that it was emotion/server that was responsable of all the dead code that was shiped to the client.
Just implemented the lazy loading aproach again and, bingo.
No API change hurray.

When the problem wasn't addressed:

Your solution:

4.7.1:

Your solution:

In both cases the _app bundle size is down from ~160Kb to ~11Kb

4.7.1:

Answer 17 · 2023-03-03T09:59:47.000Z

Thanks for all the effort. When implementing the v4.7.1 i still see some unwanted behaviour.

I still think there are some unneeded packages in the _app that aren't used. I've made an comparison branch here. Where I've split the augmentDocumentWithEmotionCache and withAppEmotionCache into their own files. And this results in a much smaller _app chunk.

The code split bundlesize result:

ssr-code-split % yarn bundlesize
yarn run v1.22.19
$ /Users/michielhennekam/projects/tss-react/src/test/apps/ssr-code-split/node_modules/.bin/bundlesize
 PASS  ./.next/static/chunks/pages/_app-8511534f44c5e2f3.js: 32.47KB < maxSize 35KB (gzip) 

✨  Done in 0.87s.

The v4.7.1 bundlesize result:

ssr % yarn bundlesize
yarn run v1.22.19
$ bundlesize
 FAIL  ./.next/static/chunks/pages/_app-c9e9f789bc733662.js: 85.31KB > maxSize 35KB (gzip) 

error Command failed with exit code 1.

Answer 18 · 2023-03-03T11:23:45.000Z

Hi @michielmetcake ,
Thank you for the feedback.
I'm very surprized, I conducted carefull tests yesterday.
I compared the size of the _app bundle before any patch: 160Kb
Your solution with two functions (tss-react: "4.6.2-rc.1"): 10.26Kb
The current solution (4.7.1): 11.39Kb

I mean, before 4.7.1 all my sollutions where dumb, it didn't significantly decreced the bundle size but now, things have been tested....

Could it be that it's bundlesize that isn't evaluating properly the actual bundle size.
Or maybe you have cache and still using the old version...?

Answer 19 · 2023-03-03T12:32:11.000Z

Hi @michielmetcake , Thank you for the feedback. I'm very surprized, I conducted carefull tests yesterday. I compared the size of the _app bundle before any patch: 160Kb Your solution with two functions (tss-react: "4.6.2-rc.1"): 10.26Kb The current solution (4.7.1): 11.39Kb

I mean, before 4.7.1 all my sollutions where dumb, it didn't significantly decreced the bundle size but now, things have been tested....

Could it be that it's bundlesize that isn't evaluating properly the actual bundle size. Or maybe you have cache and still using the old version...?

Well I noticed it because we want to use this package in the production version of our app. But it's blocking us now because some files from the _app chunk are not transpiled to es5. Which I wasn't expecting because with the two split functions it's all fine.

Answer 20 · 2023-03-03T13:06:29.000Z

I went into the trouble of testing your branch: https://github.com/michielmetcake/tss-react/tree/v4.71-vs-code-split

I get the same result with code splitting and lazy loading:

Code spliting: 118Kb

Lazy loading (4.7.1): 119Kb

As far as I can see the bundle size problem is fixed.

Now what you seem to be experiencing is an other issue that I'd be happy to address in a new issue if you can provide me with a reproduction path.

I understand you'd prefer that I merge your code splitting implementation and get it over with already but let me explain why I'm trying to avoid this senario:

Pepole are not tolerant at all to breaking changes. All the more so coming from a library they use because they have no other choice. TSS shiping to the frontend a bunch of dead code is a bug, and a big one at that (thank you for spotting it!). It had to be fixed for everyone, not only the ones willing to update their setup.
The code splitting approach introduces duplication of source of truth for the key that has to be provided to both the _app and the _document.

If you can follow up by providing a reporoduction repo for the issu you are currently experiencing great. Otherwise remember that you always have the option to copy your augmentDocumentWithEmotionCache and withAppEmotionCache and paste it into your project.
After all TSS is providing an helper here just because I figured out that the logic for setting up SSR for emotion can be abstracted away but MUI and Emotion expect you to do it by hand....