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

Route Based Code Splitting #265

Merged
merged 17 commits into from
Jun 4, 2020
Merged

Conversation

hutchgrant
Copy link
Member

@hutchgrant hutchgrant commented Nov 27, 2019

Related Issue

Resolves #136

Summary of Changes

  1. Modify scaffold to build lit-redux routes using lazy loaded dynamic imports
  2. Add babel-eslint
  3. Enable @babel/plugin-syntax-dynamic-import plugin in babel config
  4. Modify webpack config to set chunk file names
  5. Fix app-template test case
  6. Add loading component and spinner

@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 27, 2019

You'll notice in the www example that since we're placing our header and footer in our page-template, we lose the SPA feel on route change with lazy loaded routes. Perhaps it might be a better idea to implement a custom app-template as part of the www as well and move the header/footer into those?

Thoughts?

Also seeing lint errors related to babel dynamic import plugin, we need an exception for that dep.

@hutchgrant
Copy link
Member Author

Added a basic loading component w/spinner

loading

@thescientist13 thescientist13 added RFC Proposal and changes to workflows, architecture, APIs, etc question Further information is requested labels Nov 27, 2019
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

There's so much cool stuff here, and I've only just given it a cursory glance so far! 💯

You'll notice in the www example that since we're placing our header and footer in our page-template, we lose the SPA feel on route change with lazy loaded routes. Perhaps it might be a better idea to implement a custom app-template as part of the www as well and move the header/footer into those?

Could you could expand on the point you made of having to lift the header and footer out of the page template? What was the reason and does this mean that each user would also have to create their own app-template.js now, in addition to page-templates?

I know for the Apollo work related to #115 there will be a similar motivation, try and push all the boilerplate into the CLI's own app-template.js so user's don't have to worry about any of that. Do you think we could achieve something similar with this?

Also seeing lint errors related to babel dynamic import plugin, we need an exception for that dep.

Did you fix these or still need help? It looks like the build is passing. (or is that was babel-eslint was for?)


One thing that might be interesting to consider is the granularity of code splitting and probably something we'll need to measure, but as expected, dynamically importing every route will naturally yield chunks per route.
Screen Shot 2019-11-27 at 2 52 48 PM

I wonder if should consider any of webpack's custom chunking capabilities to try and create chunks per page / section, and a common / shared bundle across them all? (oh boy, RegExp! 🤣 )

  • about.bundle.js
  • docs.bundle.js
  • vendor.js

Only having one bundle for all the About pages at once would be nice, no having to refetch on every subnav click and might help keep things feel more responsive overall.

One thing about what have now is that even though the bundle is 300KB, that's the entire site. No more fetching, which is definitely more along the SPA model, so maybe the real answer is somewhere in between?

For example, since you could have 100s of blog pages e.g.

pages/
  blog/
    2019/
      post-one.md
      post-two.md
      post-three.md
    2018/
      post-one.md
      post-two.md
    etc/  

So for that case, yeah, it might make sense to chunk them all out individually, especially as the site grows since there all more or less standalone documents.

But for another use case, like documentation, where the list may be smaller / fixed and the content is closely related, grouping by top level route may be more desired from a UX perspective.

Anyway, just thinking out loud. Let me know your thoughts and nice work! 💪

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/cli/package.json Outdated Show resolved Hide resolved
packages/cli/src/config/webpack.config.common.js Outdated Show resolved Hide resolved
packages/cli/src/lifecycles/scaffold.js Show resolved Hide resolved
packages/cli/src/lifecycles/scaffold.js Outdated Show resolved Hide resolved
www/components/loading/spinner.js Outdated Show resolved Hide resolved
@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 27, 2019

The lint errors were related to the dynamic-import-plugin which apparently is already included in our dependencies. The only way I found to get around that problem was to add a .eslintignore file and specified all node_modules/ files.

The app-template header/footer methodology is similar to any one page app. You want to keep the header/footer consistent, while the content is the only thing that lazyily loads. This is better UX IMO. Otherwise the entire page goes blank every single click and it defeats the purpose of an SPA.

@thescientist13
Copy link
Member

thescientist13 commented Nov 27, 2019

The app-template header/footer methodology is similar to any one page app. You want to keep the header/footer consistent, while the content is the only thing that lazyily loads. This is better UX IMO. Otherwise the entire page goes blank every single click and it defeats the purpose of an SPA.

For sure, and totally understand. I was just under the impression we were already doing something like that with this, re: <entry>

<div class='wrapper'>
  <eve-header></eve-header>

    <div class="content">
      <entry></entry>
    </div>

  <eve-footer></eve-footer>
</div>

But I do see now that is more of mechanism of our scaffolding, and not so much the runtime (e.g. routing), where as you point out, in a SPA, you would have something like:

<header></header>
    
  <router-outlet></router-outlet>

<footer></footer>

With the router outlet being more a fragment / encapsulated component's HTML for just page content.

An interesting distinction / challenge I see now. Will think about some of this myself as I continue to familiarize myself with working on #115 as well, since we are now seeing similar architectural needs converging at once on this. (as you anticipated).

@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 27, 2019

What if we set code splitting as toggleable from the greenwood config, include a default loading component, allow that to be overriden the same way we do app-template? We would need to add logic to the context and config lifecycles for that, as well as a condition to the scaffold, but it's easily doable.

@hutchgrant
Copy link
Member Author

hutchgrant commented Dec 4, 2019

I've modified it to split into 4 main chunks, 1 for each of our main categories, as well as an additional vendor.js chunk. Obviously this only works for our website but I've done this as an example because you mentioned it would be cool to combine the pages based on the top-level route.

The tests failed because now there is an additional script element. Do you want to keep the vendor.js bundle?

But for another use case, like documentation, where the list may be smaller / fixed and the content is closely related, grouping by top level route may be more desired from a UX perspective.

That's what I've done in this latest commit. Test it out. I noticed the docs bundle is showing up on other unrelated routes which I don't get but for the most part it works.

test(module, chunks) {
const regexp = /about/i;
const result = `${module.name || ''}`.match(regexp) || chunks.some(chunk => `${chunk.name || ''}`.match(regexp));

Copy link
Member Author

@hutchgrant hutchgrant Dec 4, 2019

Choose a reason for hiding this comment

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

Yes it's hardcoded into 4 cache groups, but ideally we could generate cacheGroups based on the graph's top-level routes. This is utilizing our newly generated chunk names page--[mainroute]--[page-name] which we create when the graph is filled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or better yet, why don't we just move this to the greenwood.config.js and allow people to set their own chunks based on our generated graph chunk names.

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting you feedback here.

I think it's great to be trying different configurations, but I would just request that for each permutation, we're running some performance benchmarks each time. I like to capture (as screenshots):

  • Network calls (e.g. network tab in your browser) to see measure number of calls, and total bundle sizes
  • Lighthouse report, ideally I would think with code splitting, we should be able to finally hit 💯 for perf. If not, we should try and understand what is holding us back
  • Bundle Analyzer report, to measure impact on bundle sizes

This is really helpful for tracking / visualizing the impact on each change. I see this permutations:

  1. Top level bundling, e.g. about.js
  2. Top level bundling + vendor chunk, e.g. vendor.js
  3. Per page level bundling, e.g. about.community.js
  4. Per page level bundling + vendor chunk, e.g. vendor.js

Although not something I am in a rush to expose yet, but something I can track in Trello and we can chat about, could be a "depth" option to let the user apply a custom chunk optimization.

So given the follow structure:

pages/
   blog/
     2019/
       one-post.md
     2018
        two-post.md

Depth would by default be 1, which would yield the following bundle(s)

  • blog.bundle.js

A depth or 2 would be:

  • blog.2019.bundle.js
  • blog.2019.bundle.js

If nothing (null) is passed, then it would just create a bundle for every import().

Anyway, good stuff, keep it up!

maxInitialRequests: 4,
automaticNameDelimiter: '~',
automaticNameMaxLength: 30,
cacheGroups: {
Copy link
Member

Choose a reason for hiding this comment

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

yeah, We should be doing this in webpack.config.common.js for the user by default.

And also do some sort of dynamic creation of cacheGroups here like we do in webpack.config.common.js with NormalModuleReplaclementPlugin.

Copy link
Member Author

@hutchgrant hutchgrant Dec 10, 2019

Choose a reason for hiding this comment

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

I was thinking along the lines of similar to a plugin. If a developer wants custom split routes, they can configure it however they please. But I also agree we should be doing it by default as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I would still like to see this done dynamically generated based on the folders set in the users pages/ directory for now. Easy enough for us to make another issue for thinking about how to make it a public API, likely hand in hand with being able to allow our Graph to also go "deeper" than a depth of one:

packages/cli/src/lifecycles/config.js Outdated Show resolved Hide resolved
@thescientist13 thescientist13 mentioned this pull request Mar 18, 2020
5 tasks
@thescientist13 thescientist13 self-assigned this Apr 28, 2020
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Cool, so yeah, still heading in the right direction, but let's veer away from worrying about making this a public API just yet, and focus on just getting this working dynamically for www/.

Will be curious to see what we can do about trying to not have to throw out the baby with the bath water regarding have to make our own custom app-template.js. 🤔

Aside from that and offloading the spinner / "suspense" implementation to another issue / PR, I am also seeing this console error when loading the Netlify branch preview.
Screen Shot 2020-04-30 at 9 10 47 PM

Let's plan to catch up on this and walk through it all together on our next meeting.


As an aside, can you please try and throttle your editor's formatter? This issue with formatting comes up a lot and it's distracting tbh. 3 (almost 4) files are purely just formatting; that's 20% of this PR's changed files. 😳

Please take care when submitting the PR to review the diff and try and roll these back if possible. Thank you. ✌️

maxInitialRequests: 4,
automaticNameDelimiter: '~',
automaticNameMaxLength: 30,
cacheGroups: {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I would still like to see this done dynamically generated based on the folders set in the users pages/ directory for now. Easy enough for us to make another issue for thinking about how to make it a public API, likely hand in hand with being able to allow our Graph to also go "deeper" than a depth of one:

.eslintignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/cli/src/config/babel.config.js Outdated Show resolved Hide resolved
packages/cli/src/lifecycles/config.js Outdated Show resolved Hide resolved
www/templates/app-template.js Outdated Show resolved Hide resolved
packages/cli/src/config/webpack.config.common.js Outdated Show resolved Hide resolved
packages/cli/src/lifecycles/graph.js Show resolved Hide resolved
packages/cli/src/lifecycles/graph.js Show resolved Hide resolved
packages/cli/src/templates/app-template.js Outdated Show resolved Hide resolved
@thescientist13 thescientist13 removed the question Further information is requested label May 1, 2020
@thescientist13 thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) P0 Critical issue that should get addressed ASAP labels May 3, 2020
@thescientist13 thescientist13 mentioned this pull request May 7, 2020
5 tasks
@thescientist13 thescientist13 mentioned this pull request May 31, 2020
5 tasks
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Had some time to review the latest changes, looking super lean now in the branch preview environment!
Screen Shot 2020-05-31 at 9 23 00 PM

This definitely lessens the severity of #305 now, since basically every page now is its own chunk, which slims things down considerably. I think a lot of what is leftover now is actually coming from Babel / node_modules, so I think maybe I might want to look into #224 some more or #321.

One small thing, I noticed in development mode is that there is an actual bundle needed? The below (local), compared to the above (feature branch preview)
Screen Shot 2020-05-31 at 9 23 07 PM

As far as the app-template.js stuff goes, I opened issue #356 for that_

path="${file.route}"
component="eve-${file.label}"
.resolve="\${() => import(/* webpackChunkName: "${file.chunkName}" */ ${file.relativeExpectedPath})}"
></lit-route>\n\t\t\t\t`;
Copy link
Member

Choose a reason for hiding this comment

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

what's the \n\t\t\t\t for? And foe the first $ need to be escaped?

Copy link
Member Author

Choose a reason for hiding this comment

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

just for code formatting, not necessary to function.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow. Formatting in the rendered HTML? Not sure that has ever really been a concern?

Copy link
Member

@thescientist13 thescientist13 Jun 4, 2020

Choose a reason for hiding this comment

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

Ah, I see.

It is the difference between this (with)
Screen Shot 2020-06-03 at 8 49 40 PM

and this (without)
Screen Shot 2020-06-03 at 8 49 49 PM

I don't think it's a big deal, and we can probably omit it. I think if anything, we should / could use a html minfier on the final output just save even more bytes. I'll handle removing the formatting and tracking html minification.

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Looks like this brings our Lighthouse score up 6 points! 🚀

Current

Screen Shot 2020-05-31 at 9 33 36 PM

This PR

Screen Shot 2020-05-31 at 9 33 44 PM

SEO score is due to branch preview environment. Also, I think our scores are worse due to Lighthouse always refining its metrics, so over time your score can go down, unless you keep up. Should definitely look into getting a 💯 ASAP though.

Gatsby Home Page (for comparison)

Screen Shot 2020-05-31 at 9 39 05 PM

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

One nice tweak might be to add a min height to the content "body" so the footer stays closer to bottom instead of jumping from the header when the page content loads.

Maybe wrap the content in div?

  render() {
    return html`
      <div class='wrapper'>
        <eve-header></eve-header>
        <div class="content-outlet">
          MYROUTES
          <lit-route><h1>404 Not found</h1></lit-route>
        </div>
        <eve-footer></eve-footer>
      </div>
    `;
  }

And update it in styles/theme.css?

@@ -23,6 +23,13 @@ connectRouter(store);

class AppComponent extends LitElement {

static get styles() {
Copy link
Member

Choose a reason for hiding this comment

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

👌

@thescientist13 thescientist13 mentioned this pull request Jun 4, 2020
5 tasks
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Nice work, and footer is looking good 👍

@thescientist13 thescientist13 merged commit 73547bd into master Jun 4, 2020
@thescientist13 thescientist13 deleted the feat/issue-136-code-splitting branch June 4, 2020 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI enhancement Improve something existing (e.g. no docs, new APIs, etc) P0 Critical issue that should get addressed ASAP RFC Proposal and changes to workflows, architecture, APIs, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Route Based Code Splitting
2 participants