Add assets to entries sorted according to their dep graph. #8092

jquense · 2018-09-12T18:44:44Z

This ~~steals~~ borrows the chunk sorting algo from html-plugin. At the moment assets are included in sort of a random order which doesn't work for css especially. There is a bunch here that i'm a little iffy on but in my tests it seems to work...

jquense · 2018-09-12T18:45:19Z

packages/gatsby/cache-dir/static-entry.js

+
+    return {
+      js: stats.assets.js.filter(isNotRootAndMatchesPage),
+      css: stats.assets.css.filter(isNotRootAndMatchesPage),


i removed the sort by preload because it's not always stable :/ so messes up the order

jquense · 2018-09-12T18:47:23Z

packages/gatsby/src/webpack/stats-extractor-plugin.js

+            })
+
+            if (groups[chunkName]) {
+              Object.entries(groups[chunkName].childAssets).forEach(


it is very unclear when this is populated, iv'e tried a bunch of import() calls to stuff and never seen this have anything in it..

jquense · 2018-09-12T18:47:40Z

packages/gatsby/src/webpack/stats-extractor-plugin.js

+
+        const webpackStats = {
+          ...jsonStats,
+          assetsByChunkName,


idk if this is still needed

DSchau

Thanks for the PR, per usual :) From what I can tell, this looks good, but I'm going to ask a few questions so I can make sure we're on the same page.

I'll probably want to pull this down and write some tests against it to validate.

Speaking of... the unit tests in Travis are failing, so those are probably worth looking into.

@pieh want to weigh in here?

packages/gatsby/src/webpack/stats-extractor-plugin.js

DSchau · 2018-09-13T14:19:58Z

packages/gatsby/cache-dir/static-entry.js

+        as="script"
+        rel={script.rel}
+        key={script.file}
+        href={`${__PATH_PREFIX__}/${script.file}`}


Does script.file === script.name here? Why the switch?

I'm presuming it's because you changed the algo in assetsForCurrentPage() so it's returning a slightly different structure?

I moved some of the work that was done here to the stats plugin, which creates an asset object like: { file, chunkName, rel }, file felt more descriptive i guess esp, next to the added "chunkName".

DSchau · 2018-09-13T14:33:56Z

packages/gatsby/src/webpack/chunk-sorter.js

+  chunk. Thus topological sorting orders chunks from bottom-layer chunks to
+  highest level chunks that use the lower-level chunks.
+*/
+module.exports = (chunks, chunkGroups) => {


100% out of scope, but I wish we could create a utility containing this algo, and then have both us and html-webpack-plugin use it 🙃

DSchau · 2018-09-13T14:35:36Z

packages/gatsby/src/webpack/stats-extractor-plugin.js

+  }
+
+  apply(compiler) {
+    compiler.hooks.done.tapPromise(


So this was tapAsync previously, now tapPromise?

I'm not a webpack guru, so not sure if it it's a meaningful difference 🤷‍♂️

same thing, the only difference is you can use a promise instead of a callback, which is more inline with what we do elsewhere

Didn't look closely enough to see if it was a refactor or a straight copy 🙃 Makes sense!

DSchau · 2018-09-13T14:47:42Z

packages/gatsby/cache-dir/static-entry.js

+  // Add <link>s for styles that should be prefetched
+  // otherwise, inline as a <style> tag
+  headComponents.push(
+    ...styles


Overall, what you're doing here is pushing non-prefetch elements to the start of the array, then everything else, correct?

What if we re-wrote this slightly for clarity (or at least I think it's clearer), e.g.

const prefetchedAssets = styles .filter(s => s.rel === `prefetch`) .map(style => ( <link as="style" rel={style.rel} key={style.file} href={`${__PATH_PREFIX__}/${style.file}`} /> )) const nonPrefetchedAssets = styles .filter(s => s.rel === `prefetch`) .map(style => ( <style key={style.file} data-href={`${__PATH_PREFIX__}/${style.file}`} dangerouslySetInnerHTML={{ __html: fs.readFileSync( join(process.cwd(), `public`, style.file), 'utf8' ) }} /> )) headComponents.unshift(...nonPrefetchedAssets) // this clarifies things a bit, i.e. that these are first headComponents.push(...prefetchedAssets)

DSchau · 2018-09-13T14:48:55Z

packages/gatsby/src/utils/webpack.config.js

-              )
-            },
-          },
+          new StatsExtractorPlugin(),


I like this change a lot! Extracting to a separate file is a nice win.

jquense · 2018-09-13T15:01:24Z

yeah i've been meaning to fix the tests, just wanted to get some feedback first before workin on it

jquense · 2018-09-17T21:04:14Z

updated, rebases and fixed tests. Anyone want to take a chance on this ;) I could use the fix :0

jquense · 2018-09-17T21:09:41Z

https://react-bootstrap.netlify.com/components/badge/

jquense · 2018-09-18T16:19:30Z

I've spent a bit more time on this and i've got a few questions...why are we auto sorting the inline styles before everything? Consider linking font-awesome and bootstrap.css, we definitely want those to show up first, before any app styles. At the moment that's pretty hard to do without implementing onPreRenderHTML despite it being a very common pattern. Ya'll need some people who use css :P

m-allanson · 2018-09-18T17:15:31Z

Hmm yeah I was looking at this earlier (using the demo repro at #6302) and noticed similar. Do you have time to make a basic repro site using plain CSS and gatsby-starter-hello-world? Then we can take that and write tests from it.

jquense · 2018-09-18T17:18:16Z

yup should have a moment today to do it

jquense · 2018-09-18T18:31:07Z

err, a quick try didn't have much luck. This is pretty hard to reproduce on a simple site. Reasons, i think, because there isn't enough complexity in the css requires, and this issue is sort of by definition non-deterministic...

m-allanson · 2018-09-18T22:38:40Z

Here's a kinda demo that shows the auto-sorting inlining styles resulting in different styles being applied in gatsby develop vs gatsby build. https://github.com/m-allanson/gatsby-v2-css-ordering.

It doesn't demo the main issue that you're fixing though.

Edit: pushing all the styles straight to headComponents will fix the behaviour in the above demo.

KyleAMathews · 2018-09-18T23:59:42Z

People use CSS still??? :-P

jquense · 2018-09-20T14:13:53Z

another data point, i've got this in react-bootstrap as a hacky plugin and it seems to work great: https://github.com/react-bootstrap/react-bootstrap/tree/master/www/plugins/gatsby-plugin-sorted-assets

jquense · 2018-09-20T14:15:01Z

and here is a great example of how it was broken prior: https://5ba00f734ed62f7dda3cf9e5--react-bootstrap.netlify.com/components/badge/

dustinhorton · 2018-11-08T21:09:17Z

Any updates here? Going to try the aforementioned plugin in the meantime—currently blocking a launch for me.

dustinhorton · 2018-11-17T02:39:21Z

#8560 possibly related. also several others issues with other weirdness of styles popping up this week.

princefishthrower · 2018-11-19T12:42:25Z

If it helps, I've also seen this issue, similarly as described by @topherauyeung:

If I land directly or refresh a page... the CSS is broken for those components. The classes are present in the source but they aren't applied to the elements. If I follow a to the same page, however, everything works fine.

This is the same for me, when I go to non-homepage links directly, the styles are wonky.

HOWEVER (possibly the helpful part) When I hover over a <Link> component on such a page, (namely the title of my blog back to the homepage, all the styles are injected and it looks as expected). So something in is triggering the styles to load correctly.

Feel free to take a look at a live version of this behaviour here:

https://chrisfrew.in/getting-started-in-2018-with-magento-apache-and-php/

just hover the mouse over anywhere on the top of the post to see the styles injected (white background should go to gray, code highlights work, etc)

dustinhorton · 2018-11-19T23:40:55Z

@frewinchristopher can see it even clearer in yours—thanks for sharing.

dustinhorton · 2018-11-27T21:59:33Z

we've had to resort to not using gatsby-link, which is a huge loss in regards to perf.

robertcoopercode · 2018-12-03T01:28:32Z

I left a detailed comment on issue #9911 mentioning how I reproduced this issue along with a repository & live demo of the issue. It's very similar to @frewinchristopher's description of the improper styles loading and taking precedence when a link is being hovered over.

jquense · 2018-12-03T01:43:33Z

I think the Gatsby link issue is slightly different than what is being solved here. That problem is that the resources for each page are not the same as what's needed for the next navigation. From what i can tell when you hover a link it preloads the next page's assets. However in the case of CSS, that's likely to include a bunch of styles that are already loaded, e.g. global site styles.

The problem with this is it realllly messes with the cascade since files that are supposed to loaded first are being loaded again after more specific styles. this doesn't always work well. What should happen is that Gatsby should only load new style resources, not all resources for the page. @pieh as far as you know does that describe the current behavior of gatsby-link, and the page resource pipeline as a whole?

dustinhorton · 2018-12-03T02:35:03Z

@jquense Agreed this PR is not solving the issue I was experiencing—meant to add that explanation to my comment @ #8092 (comment). I thought it was happening on load and not just after hovering a gatsby-link at some point, but as of now it's only the latter.

What should happen is that Gatsby should only load new style resources, not all resources for the page.

I think that's correct. I thought I could get around this by importing global styles to gatsby-browser.js, but that didn't fix the problem.

Off the top of my head, other ways to fix might be 1. doing whatever happens in dev because it doesn't happen there 2. disable css prefetching (or make it disable-able).

pieh · 2018-12-03T02:44:35Z

The problem with this is it realllly messes with the cascade since files that are supposed to loaded first are being loaded again after more specific styles. this doesn't always work well. What should happen is that Gatsby should only load new style resources, not all resources for the page. @pieh as far as you know does that describe the current behavior of gatsby-link, and the page resource pipeline as a whole?

We don't inject same styles again. This is different problem - see #9911 (comment)

---edit:
I should rather say - we don't inject same stylesheet files. In this case styles are duplicated in multiple files causing injection of same styles.

dustinhorton · 2018-12-03T03:31:41Z

@pieh Did you get a chance to check out my repro repo? https://github.com/dustinhorton/marketing

Just realized the docs (https://www.gatsbyjs.org/docs/creating-global-styles/) still recommend loading global styles in the layout component, if it exists—I could revert to that.

jquense · 2018-12-03T04:04:43Z

I should rather say - we don't inject same stylesheet files. In this case styles are duplicated in multiple files causing injection of same styles.

yeah, i think i had noticed tho that the way webpack splits css chunks with the SSR setup you get a lot duplication across chunks. e.g. the stylesheet is different (page--foo, page--bar.css, etc) but much of the content is the way. Might need a more aggressive splitChunks setup perhaps?

jquense · 2018-12-19T16:32:38Z

any chance we could merge this? I relize it may not solve all the problems but it definitely solves some of them. I'm tired of copying this plugin into every site i build :P

aspiers · 2019-01-06T16:20:11Z

I would love to see this merged too. Seems like it's affecting several people. I just tried rebasing it and it rebased cleanly against master, as shown in master...aspiers:sort-chunks. @jquense are you able to rebase and force-push?

princefishthrower · 2019-01-12T21:59:34Z

For what it's worth, I have a workaround for the time being, which is simply loading any custom CSS from index.html To do this in Gatsby, just follow this page to create a proper html.js file, then customize it with a nice <link> tag in the <head>, ex:

<link rel="stylesheet" type="text/css" href="https://yoursitehere.com/styles.css"/>

Make sure your style file(s) are in your static/ folder (in this case, the root of the static/ folder) before you issue gatsby build! It's not a very clean solution, but it works for the time being.

KyleAMathews · 2019-02-15T17:49:42Z

@jquense is this still needed w/ #11072

DSchau · 2019-04-08T19:32:29Z

This PR has gotten in a pretty gnarly state, and I think it's extremely plausible that it's not needed with #11072.

As such--let's close this out. If I'm off base or if this is still worth re-visiting, let one of us know or re-open and we'll take another look.

As always, thanks @jquense for the PR. Sorry this didn't get merged!

jquense · 2019-04-08T19:59:13Z

hmm #11072 seems like a worse solution to this? This is already a solved problem webpack html plugin does this and its what most apps use as a default. not splitting css seems like a worse default.

jquense · 2019-04-08T20:01:30Z

also this handles js assets as well, which should also be sorted and included in the right order

DSchau · 2019-04-08T20:03:40Z

@jquense let's revisit this, then.

When you get a minute, want to fix up the merge conflicts? Otherwise I'm happy to do this later, as well.

jquense · 2019-04-08T20:04:05Z

ya definitely i'll take some time to clean it up 👍

freiksenet · 2019-06-10T14:19:14Z

Hi @jquense!

We are going through old PRs with the core team. We really appreciate this contribution and we would like to have it in the Gatsby repo eventually. However, as there haven't any activity with the branch lately, we are going to close it.

Feel free to reopen it when you get the time to fix the merge conflicts.

Thank you so much for your contributions and looking forward for more of them. 💜 💪

jquense · 2019-06-10T15:01:30Z

i've published a hacky third party version at: https://www.npmjs.com/package/gatsby-plugin-sorted-assets

Ya'll sorry i haven't had time to update this! I probably won't have more time in the near future but i do want to nominate this issue as something that the team should explicitly mark as needed. The current way gatsby handles adding css assets is broken, and perhaps the severity of it is offset by folks using css-in-js solutions, but topo-sorting the css files is essentially required for css-modules at least, and is way webpack-html-plugin does it as a default. 👍

wildeyes · 2019-09-15T15:25:34Z

Sadly, @jquense the plugin doesn't seem to work with gatsby@^2.15.15 :(

jquense requested a review from KyleAMathews September 12, 2018 18:44

jquense commented Sep 12, 2018

View reviewed changes

jquense force-pushed the sort-chunks branch from 12a7a55 to 88a281c Compare September 12, 2018 18:48

DSchau reviewed Sep 13, 2018

View reviewed changes

KyleAMathews force-pushed the master branch 5 times, most recently from fc4ca3e to 2116fff Compare September 17, 2018 20:36

jquense force-pushed the sort-chunks branch 2 times, most recently from 80f3bfe to 6436ce9 Compare September 17, 2018 20:59

jquense added 2 commits September 17, 2018 17:02

add asset sorting

5c53f1a

address feedback

6f7c669

jquense force-pushed the sort-chunks branch from 6436ce9 to 6f7c669 Compare September 17, 2018 21:03

jquense added 2 commits September 19, 2018 10:05

fix bugs in sorter

8a3cd5b

push all styles in-order

148a166

DSchau mentioned this pull request Sep 27, 2018

gatsby build results in different CSS to that of gatsby develop #8560

Closed

kakadiadarpan mentioned this pull request Nov 6, 2018

CSS order messed up after build #9733

Closed

kevinmichaelchen mentioned this pull request Dec 19, 2018

Flicker of Unstyled Content #5667

Closed

dustinhorton mentioned this pull request Feb 16, 2019

fix(gatsby): create a single CSS file to avoid code-splitting issues #11800

Merged

DSchau closed this Apr 8, 2019

DSchau reopened this Apr 8, 2019

pieh mentioned this pull request Apr 19, 2019

Fix styles to be sorted on static-entry.js #13482

Closed

DSchau added the status: awaiting author response Additional information has been requested from the author label May 21, 2019

freiksenet closed this Jun 10, 2019

MichaelDeBoey deleted the sort-chunks branch January 20, 2020 18:15

Add assets to entries sorted according to their dep graph. #8092

Add assets to entries sorted according to their dep graph. #8092

Conversation

jquense commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquense commented Sep 13, 2018

jquense commented Sep 17, 2018

jquense commented Sep 17, 2018 • edited Loading

jquense commented Sep 18, 2018

m-allanson commented Sep 18, 2018

jquense commented Sep 18, 2018

jquense commented Sep 18, 2018

m-allanson commented Sep 18, 2018 • edited Loading

KyleAMathews commented Sep 18, 2018

jquense commented Sep 20, 2018

jquense commented Sep 20, 2018

dustinhorton commented Nov 8, 2018

dustinhorton commented Nov 17, 2018

princefishthrower commented Nov 19, 2018 • edited Loading

dustinhorton commented Nov 19, 2018

dustinhorton commented Nov 27, 2018

robertcoopercode commented Dec 3, 2018

jquense commented Dec 3, 2018 • edited Loading

dustinhorton commented Dec 3, 2018

pieh commented Dec 3, 2018 • edited Loading

dustinhorton commented Dec 3, 2018

jquense commented Dec 3, 2018

jquense commented Dec 19, 2018

aspiers commented Jan 6, 2019 • edited Loading

princefishthrower commented Jan 12, 2019 • edited Loading

KyleAMathews commented Feb 15, 2019

DSchau commented Apr 8, 2019

jquense commented Apr 8, 2019

jquense commented Apr 8, 2019

DSchau commented Apr 8, 2019

jquense commented Apr 8, 2019

freiksenet commented Jun 10, 2019

jquense commented Jun 10, 2019

wildeyes commented Sep 15, 2019

DSchau left a comment •

edited

Loading

jquense commented Sep 17, 2018 •

edited

Loading

m-allanson commented Sep 18, 2018 •

edited

Loading

princefishthrower commented Nov 19, 2018 •

edited

Loading

jquense commented Dec 3, 2018 •

edited

Loading

pieh commented Dec 3, 2018 •

edited

Loading

aspiers commented Jan 6, 2019 •

edited

Loading

princefishthrower commented Jan 12, 2019 •

edited

Loading