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

Client side caching enabled. #1806

Merged
merged 9 commits into from Oct 2, 2019
Merged

Conversation

revanth0212
Copy link
Contributor

@revanth0212 revanth0212 commented Sep 27, 2019

Description

This is a follow up to #1691. That PR was causing issues when running in the watch mode. This PR addresses most of the changes addressed in that PR without service worker changes. For info about why these changes, check #1691

Related Issue

Closes #1623.

Verification Steps

  1. Build the venia code base using yarn build.
  2. Stage the newly created build using yarn stage:venia.
  3. Open a browser tab with cache cleared or try using an incognito tab.
  4. Open network dev tools tab.
  5. Now go to the venia instance using the URL that was provided in the terminal during stage process.
  6. The first time you should see client.[hash].js, runtime.[hash].js, vendors.[hash].js, registerSW.[hash].js and sw.[hash].js downloaded apart from other files that are not of significance in this PR.
  7. When you refresh the UI again, no files have to redownload. Meaning they have been cached the first time.
  8. Now stop the staging server and run steps 1, 2 and 5 again. This time again since the code has not changed the build should not change and nothing should be downloaded from the server.
  9. Again stop the staging server, try changing couple lines of code and redo steps 1, 2 and 5. This time only the client.[hash].js should be downloaded because the app build has changed.

Screenshots / Screen Captures (if appropriate)

Check out #1623 for screenshots and metrics.

Checklist

  • App logic should not have changed. These are code build changes.
  • Should work in both watch and stage` mode.

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Sep 27, 2019
@revanth0212 revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Sep 27, 2019
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Sep 27, 2019

Fails
🚫 Issue 1623 is closed. Please make sure the linked issue is correct and open.
Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 4870e18

@revanth0212
Copy link
Contributor Author

One weird thing I have noticed with webpack is, after these changes for some reason the client.js file's size grew by 4KB. Even though it is a small amount, it is a point to be noted.

Before Change:
image

After Change:
image

@supernova-at supernova-at moved this from Ready for Review to Review in Progress in Pull Request Progress Oct 1, 2019
@supernova-at supernova-at self-assigned this Oct 1, 2019
@supernova-at
Copy link
Contributor

after these changes for some reason the client.js file's size grew by 4KB. Even though it is a small amount, it is a point to be noted.

Hmm, yeah - thanks for pointing that out. Not sure why that would be, I don't see anything obvious here 🤔 .

@supernova-at
Copy link
Contributor

  1. Again stop the staging server, try changing couple lines of code and redo steps 1, 2 and 5. This time only the client.[hash].js should be downloaded because the app build has changed.

I initially changed packages/venia-ui/lib/RootComponents/CMS/cms.js (so I could verify that my changes took). This didn't cause client.[hash].js to be downloaded - instead, the CMS Root Component and the runtime.[hash].js were downloaded:

Screen Shot 2019-10-01 at 11 38 52 AM

Note that these have the gear icon to indicate they were initiated by the service worker.

When I updated a non-Root component (packages/venia-ui/lib/components/CategoryTree/categoryLeaf.js), I did see an update only to client.[hash].js though, so I think RootComponents are just a special case 👍 .

* needs to be downloaded. Separating them will only
* download runtime bundle and use the cached client code.
*/
runtimeChunk: 'single',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to change the name of runtime ? Since it's specific to webpack it'd be nice if it was webpack-runtime or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me look into that. Not sure if that is possible.

@supernova-at supernova-at moved this from Review in Progress to Reviewer Approved in Pull Request Progress Oct 1, 2019
@dpatil-magento dpatil-magento moved this from Reviewer Approved to QA In Progress in Pull Request Progress Oct 1, 2019
@m2-community-project m2-community-project bot moved this from QA In Progress to Review in Progress in Pull Request Progress Oct 1, 2019
@dpatil-magento dpatil-magento moved this from Review in Progress to QA In Progress in Pull Request Progress Oct 1, 2019
@dpatil-magento dpatil-magento merged commit 5627689 into develop Oct 2, 2019
@dpatil-magento dpatil-magento deleted the revanth/webpackCaching branch October 2, 2019 14:55
@m2-community-project m2-community-project bot moved this from QA In Progress to Done in Pull Request Progress Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:pwa-buildpack version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

[Research Story]: Audit Build Settings for Performance Improvements
4 participants