gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.3k stars 10.31k forks source link

Generate new two version of javascript bundle (app-hash,js) #24906

Closed ghassanmas closed 4 years ago

ghassanmas commented 4 years ago

Summary

This feature is relevant for the build stage So Gatsby creates a JS bundle which will be used:


### Motivation

I think this would decrease the file size sent to the client, which would lead to better performance. 
I have encountered that when testing with lighthouse, hence I got unused script warning, this is a related issue #24332. (I have used [webpack bundle analyzer](https://www.gatsbyjs.org/packages/gatsby-plugin-webpack-bundle-analyser-v2/) also the [dev tools coverage tools](https://developers.google.com/web/tools/chrome-devtools/coverage)) which eventually lead me to this conclusion. 
herecydev commented 4 years ago

Couldn't you push that logic into node creation, so that you're querying "pre-formatted" words? Do you have a concrete example?

ghassanmas commented 4 years ago

@herecydev could you elaborate more please, I didn't quite get your question...

herecydev commented 4 years ago

Well if you need something only during building, could you not do that work when creating graphql nodes and then just query the output of that? Then your react code doesn't need to rely on @formatjs and it won't be bundled

ascorbic commented 4 years ago

I may be missing something, but the problem I see here is that any package used in HTML generation is going to be needed when navigating between pages, as the page is generated dynamically then using the same code. It's also likely to be needed at rehydration time. Can you describe a case when this wouldn't be the case? Off the top of my head, you could in theory use an onCreateWebpackConfig hook to replace the loader for that module with the null loader if it's in the build-javascript phase. Not sure if that would work though.

ghassanmas commented 4 years ago

@ascorbic @herecydev I certainly need the packages to be used in the rehydration time (SSR). So telling the webpack to ignore loading the modules/packages would throw an error.

The thing is I need gatsby to use these packages in the rehydration time only. after that, the packages are irrelevant to be used/served to the client in the browser runtime environment.

ascorbic commented 4 years ago

OK, but they need to be served to the browser in order to run at hydration time, so surely it's a moot point. They'll already be loaded and cached. If they're used on more than one page they'll be in a separate chunk so won't need loading again.

ghassanmas commented 4 years ago

@ascorbic correct if I am wrong please As to my understanding, the hydration occurs when gatsby is building/generating the HTML pages. So I don't get it your point that:

they need to be served to the browser in order to run at hydration time.

Can you elaborate more please, and thanks for you patience.

ascorbic commented 4 years ago

Hi. No, the SSR happens at build time, but hydration happens after the page is loaded. @joshwcomeau did a great post explaining it.

ghassanmas commented 4 years ago

@ascorbic I stand corrected then. The packages/modules are needed only in the SSR process. I just checked the source content of the HTML generated in the build phase. And it seems the module did its job in the SSR phase, so serving it to the client is irrelevant.

ascorbic commented 4 years ago

OK, but as part of hydration, React needs to be able to generate the whole page again on the client side. Surely that means it will need those packages. Also when you navigate between pages it doesn't load the SSRd HTML, but rather just the page data, and renders it on the client. The scenarios where there will be packages used during SSR that are not needed during hydration are very rare.

ghassanmas commented 4 years ago

@ascorbic

but as part of hydration, React needs to be able to generate the whole page again on the client side. Surely that means it will need those packages

Are you saying then ReactDOM hydration is as computationally expensive as ReactDOM render?... When I read the article you referenced above as well as react docs that wasn't the case to my understanding... it seemed that react hydration job is mainly to add attach the events to the window objects...

Also when you navigate between pages it doesn't load the SSRd HTML, but rather just the page data, and renders it on the client.

It doesn't load the HTML it at all? Can you refer me to gatsby doc where it talks about it. On the other hand if that is true, doesn't it downgrade the main value of gatsby pre generating the HTML pages

The scenarios where there will be packages used during SSR that are not needed during hydration are very rare.

while may not :100: true for my personal case (I still need to do deeper analysis and debugging to confirm)., however I still believe its gotta be an essential feature to have for gatsby, given it affect the lighthouse score as mentioned above #24332 .

ascorbic commented 4 years ago

No, it's not as expensive because it doesn't need to update the DOM apart from attaching listeners. That means it's a lot cheaper. The SSR is to improve the first page load. On subsequent navigation, by just loading the page data we can load far less data and it's quicker still.

I'm going to close this because I can't see a use case for it. The reality of React SSR is that you'll almost always need the same imports.

ghassanmas commented 4 years ago

@ascorbic Sorry I didn't got notified earlier about your last comments. I have submitted a use case in this issue's description. e.g. when using the formatjs.

The reality of React SSR is that you'll almost always need the same imports.

On the other hand having you said "Almost" implies that are cases that where imports are not needed in the client as well. Please reopen this issue to keep the discussion going about this important topic!.

ghassanmas commented 4 years ago

There is also a very equivalent to a serious issue taken by next js' development team.
Check this article https://arunoda.me/blog/ssr-and-server-only-modules and this issue

herecydev commented 4 years ago

I'm not sure comparing next.js issues is helping the issue here. Do you have an example (or can put together one) where you have something that is only used for SSR and shouldn't be included in a browser bundle? Otherwise we're just dealing in hypotheticals

ghassanmas commented 4 years ago

Again, let put my example in details, When using a i18n library where you need to insert only the values at render time, not event listener are needed. Why I would need to use the module in the client side if the module doesn't attach event listeners or has no side effect relative user interaction. Only needed to when for doing the first render.

Next.js is very similar Gatsby, both projects can learn from each other. This is the spirit of open source.

herecydev commented 4 years ago

When using a i18n library where you need to insert only the values at render time, not event listener are needed. Why I would need to use the module in the client side if the module doesn't attach event listeners or has no side effect relative user interaction. Only needed to when for doing the first render.

Can you put together an actual reproduction (code) of that problem? React's hydration process mandates that they are the same thing:

React expects that the rendered content is identical between the server and the client. It can patch up differences in text content, but you should treat mismatches as bugs and fix them

If you can create a small reproduction that we can then use as a reference that'll be very helpful

ghassanmas commented 4 years ago

@herecydev sounds fair, I will create a small gatsby project and link it here then.