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

Webpack HMR, Add hashes to chunk filenames. #878

Merged
merged 43 commits into from
Jun 2, 2020
Merged

Conversation

timgl
Copy link
Collaborator

@timgl timgl commented May 27, 2020

Changes

  • According to this comment this might help, but I wasn't able to reproduce the actual issue.
  • Added benefit might be that this works as a cache buster due to the hash?

Checklist

  • All querysets/queries filter by Team (if applicable)
  • Backend tests (if applicable)
  • Cypress E2E tests (if applicable)

@timgl timgl requested a review from mariusandra May 27, 2020 17:22
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 27, 2020 17:23 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 09:27 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 09:33 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 11:38 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 11:41 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 11:45 Inactive
@mariusandra
Copy link
Collaborator

Hi! I added a bunch of changes to this PR, including HOT reloading:

screencast 2020-05-28 13-46-20

(so this fixes and closes #877, #867, #691, #686)

Regarding chunks:

  • I changed the hashing policy to contenthash from chunkhash and added the hashes to CSS and images as well.
  • I think contenthash is better for caching, as this hash is calculated 100% based on the content of the file. There's also hash, which is unique per build and chunkhash, which is unique per build per chunk. We don't need that level of granularity in the CDN.
  • We don't really use a CDN, so these "chunk loading" errors will still happen. Basically they happen when user X is on version "N - 1" of posthog and we just deployd "N". When they click on a link in the sidebar, they will want to load the chunk for version N-1, but it's no longer available. We should try to be smart in this case and reload the entire app... or at least give the user a good error for what happened.
  • Right now all static files are served through django from the generated static folder, so it'll just give a 404 when loading an old chunk.
  • If we would use a CDN that would cache all the JS files that are generated on every build, including e.g. all the "trends.HASH.js" files, the user would keep using the older version of the app and be none the wiser... until they run across a backwards incompatible change in an API that would crash the older version.
  • There's also a pattern I've seen where the app itself shows a popup of "new version available! hit reload", which can get really annoying for really small features that are deployed multiple times per day. There's probably a reason I've never seen such a popup on e.g. Facebook.

Regarding HMR (Hot Module Reloading:

  • We now have webpack-dev-server running on port 8234 and serving the latest JS + HMR updates
  • I added a webpack-html-plugin to generate an index.html with the right initial scripts (including hash!) and in dev mode the paths will resolve to the dev server
  • There is an issue with code splitting and HMR. The fix right now is to wrap each exported scene with a hot(Events) wrapper. It's not ideal, but considering potential tradeoffs I think it's good enough for now. We should just remember this when adding new scenes or the new pages won't reload (and we'll fix them then).

@mariusandra
Copy link
Collaborator

This seems to break cypress, so not yet ready to be merged. I'll see whats up.

@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 12:19 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 12:41 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz May 28, 2020 12:42 Inactive
@timgl
Copy link
Collaborator Author

timgl commented May 28, 2020

This is awesome. With the login/logout stuff, is it going to work with our production setup at http://github.com/posthog/posthog-production ?

@mariusandra
Copy link
Collaborator

It should. I just added a fix to generate a proper layout.html file that links to the right main.[contenthash].css file. It'll be in the frontend/dist folder like before and will be picked up by django's collectstatic.

So I don't see why it wouldn't work... :)

package.json Outdated Show resolved Hide resolved
@timgl timgl mentioned this pull request Jun 1, 2020
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz June 1, 2020 09:28 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz June 2, 2020 08:01 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz June 2, 2020 08:04 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz June 2, 2020 08:51 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz June 2, 2020 08:59 Inactive
@timgl timgl temporarily deployed to posthog-877-chunk-loadi-cissqz June 2, 2020 09:06 Inactive
@mariusandra
Copy link
Collaborator

mariusandra commented Jun 2, 2020

Hi, this works now and is merged with the latest master. I would keep this out of Wednesday's release as many moving parts got updated and we should test it a bit.

What got added in the end:

  • Chunk hashes in JS filenames (trends.HASH.js)
  • No hash for now in the filenames of "main.js" and "editor.js", just so the editor could work well (by just loading /editor.js and /editor.css). We can improve on this later.
  • No hash in the filenames of CSS files because of the above point.
  • HOT module reloading for React code.
  • Cypress tests now run off the production docker, not the dev docker.
  • Dev docker works with webpack-dev-server (on port 8234)
  • The index.html and layout.html files now get created with HtmlWebpackPlugin. There's just one file left in the "frontend/public" folder (posthog-logo.png). We could clean that up as well and make everything go through webpack.

Future improvements:

  • Move Cypress onto a faster system and split into parallel pipelines
  • Make sure the editor works well with HMR
  • Add hashes to editor.js and editor.css and get the right script to load via the API

@mariusandra mariusandra changed the title Closes #877 chunk loading errors Webpack HMR, Add hashes to chunk filenames. Jun 2, 2020
@timgl
Copy link
Collaborator Author

timgl commented Jun 2, 2020

I just cut a release so can merge this.

@timgl timgl merged commit bcf427c into master Jun 2, 2020
@timgl timgl deleted the 877-chunk-loading-errors branch June 2, 2020 09:39
@timgl timgl mentioned this pull request Jun 2, 2020
3 tasks
@EDsCODE EDsCODE mentioned this pull request Jun 6, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants