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

enhancement/issue 629 cache unchanged assets in development #760

Merged

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Oct 10, 2021

Related Issue

resolves #629 and #553

Summary of Changes

  1. Cache importMap from initial walking of all dependencies
  2. Set and check ETag and If-None-Match headers respectively as part of dev server request / response middleware. Also setting Cache-Control header too.
  3. Created shareable hashString function
  4. Upgraded to latest version of es-module-shims dependency
  5. Added a mention of this to the How It Works section

Benchmarks - #760 (comment)

TODOs

  1. A / B testing against master branch
  2. Figure out why Chrome doesn't seem to support this for ESM specifically? Is fine with CSS. maybe need to set a Cache-Control
  3. Should only happen during development
  4. Have packages share hashString implementation
  5. Verify if es-module-shims is causing double renders / script executions?
    • I think default behavior is working fine, so no need to mess with anything IMO

Testing / Demo

So far this works great in Firefox, but testing (as in this feature) can be thought of as a stepping stone to HMR. The value here is in seeing if development is / feels faster with these changes as opposed to not having it.

⚠️ Make sure you don't have Disable Cache set in your dev tools! ⚠️

  1. Open a new tab with the network tab open
  2. Load the website with the develop command and observe the status of everything should be 200
    Screen Shot 2021-10-10 at 12 07 58 PM
  3. Refresh and now the status should change to 304
    Screen Shot 2021-10-10 at 12 08 14 PM
  4. Change a file and observe now that there is new content, the status has changed back to 200 and the page should have changed, but everything else stays 304
    Screen Shot 2021-10-10 at 12 08 33 PM
  5. Keep refreshing, and the status should go back to 304 for everything
    Screen Shot 2021-10-10 at 12 08 45 PM

Questions

  1. Should it be configurable? Or at least documented for awareness?
    • It's always easy enough to just set_Disable Cache_ from the dev tools to bypass, which we can document as well.
  2. I notice with live reload, just saving with no changes causes a reload, I wonder if it only looks for a timestamp? Not sure if it's something we can do but it would be great if no change didn't cause a reload, like Skip file write if file content isn't changed postcss/postcss-cli#320 - added to my personal backlog
  3. Where should this happen, before or after post-processing in middleware chain? If first, everything after has to do null check on ctx.body. Maybe it's something we could apply to ResourceInterface?
  4. Should this exclude:
    • binary files
    • API calls - Yes
    • external HTTP requests - Yes
    • HTML?

@thescientist13 thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI discussion tied to an ongoing discussion or meeting notes labels Oct 10, 2021
@thescientist13 thescientist13 self-assigned this Oct 10, 2021
if (inm && inm === etagHash) {
ctx.status = 304;
ctx.body = null;
ctx.set('Etag', etagHash);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if there's a reason to not ever to set the ETag, or what the implications of setting it all the time would be? 🤔

@@ -285,7 +285,9 @@ class NodeModulesResource extends ResourceInterface {
// for each entry found in dependencies, find its entry point
// then walk its entry point (e.g. index.js) for imports / exports to add to the importMap
// and then walk its package.json for transitive dependencies and all those import / exports
walkPackageJson(userPackageJson);
if (Object.keys(importMap).length === 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if there is something more elegant other than initializing importMap = null, but effectively walking no dependencies should be almost zero cost as it is? Ideally for someone with a handful of dependencies, walking that everytime would be annoying.

Another thought, if we only need to do this once, then in theory we should also be able to include this in the should intercept logic instead?

 async shouldIntercept(url, body, headers) {
    return Promise.resolve(Object.keys(importMap).length === 0 && headers.response['content-type'] === 'text/html');
  }

@thescientist13 thescientist13 force-pushed the feature/issue-629-cache-unchanged-assets-in-development branch from 0418b18 to 43c2d75 Compare December 25, 2021 01:18
@thescientist13 thescientist13 added feature New feature or request and removed enhancement Improve something existing (e.g. no docs, new APIs, etc) labels Dec 25, 2021
@thescientist13
Copy link
Member Author

thescientist13 commented Feb 19, 2022

Benchmarking

So did some local testing with Chrome and Firefox on master vs this branch. Results are basically almost a 50% percent improvement for refreshed and live reloads, which is pretty awesome! With E-Tags, unchanged assets stay the same naturally, which you can by their E-Tag staying the same, which adds a really nice level of granularity to loading. 💪

It should be noted Firefox is pretty fast overall, compared to Chrome but everything feels a lot quicker and responsive now overall. ✨ 💯

Master

Firefox

Load: ~1.10 - 1.20s
Screen Shot 2022-02-19 at 5 41 06 PM

Refresh: 650-785ms
Screen Shot 2022-02-19 at 5 41 15 PM

Live Reload: 650-763ms
Screen Shot 2022-02-19 at 5 41 27 PM

Chrome

First Load: ~1.8 - 2s
Screen Shot 2022-02-19 at 5 46 54 PM

Manual Refresh: ~800ms - 1s
Screen Shot 2022-02-19 at 5 47 06 PM

Live Reload: ~1.9s
Screen Shot 2022-02-19 at 5 47 18 PM


Current Branch

Firefox

Load: ~800ms
Screen Shot 2022-02-19 at 5 50 56 PM

Refresh: ~380ms
Screen Shot 2022-02-19 at 5 51 13 PM

Live Reload: ~375-450ms
Screen Shot 2022-02-19 at 5 51 28 PM

Chrome

Load: ~1.83s
Screen Shot 2022-02-19 at 5 57 02 PM

Refresh: 500-600ms
Screen Shot 2022-02-19 at 5 57 13 PM

Live Reload: 450-600ms
Screen Shot 2022-02-19 at 5 58 00 PM

@thescientist13 thescientist13 force-pushed the feature/issue-629-cache-unchanged-assets-in-development branch from 43c2d75 to bfce7ce Compare February 19, 2022 23:06
@thescientist13 thescientist13 marked this pull request as ready for review February 20, 2022 17:17
@thescientist13 thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) and removed feature New feature or request labels Feb 20, 2022
@thescientist13 thescientist13 changed the title Feature/issue 629 cache unchanged assets in development enhancement/issue 629 cache unchanged assets in development Feb 20, 2022
@thescientist13 thescientist13 added the documentation Greenwood specific docs label Mar 3, 2022
@thescientist13 thescientist13 removed their assignment Mar 3, 2022
@thescientist13 thescientist13 force-pushed the feature/issue-629-cache-unchanged-assets-in-development branch from 0bd2e3c to 1ee4f54 Compare March 5, 2022 16:27
@thescientist13 thescientist13 merged commit 18cff9a into master Mar 5, 2022
@thescientist13 thescientist13 deleted the feature/issue-629-cache-unchanged-assets-in-development branch March 5, 2022 16:41
@thescientist13 thescientist13 linked an issue Mar 15, 2022 that may be closed by this pull request
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI discussion tied to an ongoing discussion or meeting notes documentation Greenwood specific docs enhancement Improve something existing (e.g. no docs, new APIs, etc)
Projects
None yet
1 participant