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

feat(dev): HMR + Hot Data Revalidation #5259

Merged
merged 67 commits into from
Feb 22, 2023
Merged

feat(dev): HMR + Hot Data Revalidation #5259

merged 67 commits into from
Feb 22, 2023

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Jan 25, 2023

Exploring the depths of our universe HMR in Remix w/ @jacob-ebey

Depends on remix-run/react-router#9996

TODO

  • reason field for HMR updates to support contextual logs and debugging
  • Prevent preload from fetching duplicate copy of updated route
  • RR: rename new API -> _internalSetRoutesAndRevalidate() (naming tbd)
  • Gate all HMR stuff behind unstable_dev future flag
  • Restore dev server ports
  • Detect loader function declaration (export function loader() {...}) OR loader variable expression (export let loader = () => {...})
  • Tests
  • move HMR from <Scripts /> to <LiveReload /> or dedicated component?
  • Resolve conflicts with dev
  • Rewrite AST transforms with babel
  • Handle MDX files in browser routes plugin
  • Changelog
  • Merge feat: mutable route tree react-router#9996
  • Point to RR pre-release (not experimental release)

Follow-on work

  • Scan dependencies and pre-bundle them
  • Optimize revalidations via granular route update APIs in RR
  • Don't send HMR-only data (like loader hashes) to browser
  • Improved error handling in compiler for esbuild's result.errors
  • Refactor compiler to have separate subcompilers for browser js and css

@changeset-bot
Copy link

changeset-bot bot commented Jan 25, 2023

🦋 Changeset detected

Latest commit: 3121ea6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
@remix-run/dev Minor
@remix-run/react Minor
@remix-run/server-runtime Minor
create-remix Minor
@remix-run/css-bundle Minor
@remix-run/testing Minor
@remix-run/cloudflare Minor
@remix-run/deno Minor
@remix-run/node Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
@remix-run/architect Minor
@remix-run/express Minor
@remix-run/netlify Minor
@remix-run/vercel Minor
@remix-run/serve Minor
remix Minor
@remix-run/eslint-config Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pcattori pcattori changed the title hmr adventures feat(dev): HMR + Hot Data Revalidation Feb 21, 2023
@pcattori pcattori merged commit 42a0817 into dev Feb 22, 2023
@pcattori pcattori deleted the pedro/hmr-adventures branch February 22, 2023 04:53
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Feb 22, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-aecf731-20230222 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Comment on lines +16 to +20
Known limitations for MVP:
- Only implemented for React via React Refresh
- No `import.meta.hot` API exposed yet
- Revalidates _all_ loaders on route when loader changes are detected
- Loader changes do not account for imported dependencies changing
Copy link
Member

Choose a reason for hiding this comment

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

Which of these are short-term limitations and which are really hard problems that will have to be solved longer-term (or never solved at all)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Revalidates all loaders on route when loader changes are detected
  • Loader changes do not account for imported dependencies changing

👆 short-term, should be resolved before HMR is stabilized

  • No import.meta.hot API exposed yet

👆 we already prototyped a import.meta.hot API so my gut is that we can also do this short-term as needed

  • Only implemented for React via React Refresh

👆 If we have import.meta.hot API, then HMR should be implementable in user-land for any other rendering frameworks/libraries. Out-of-the-box HMR support for other renderers would come after we support that renderer (e.g. after @remix-run/vue exists or something like that).

Copy link
Member

Choose a reason for hiding this comment

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

Sweet. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Also, I've noticed that HMR can be quite slow (several seconds after save). Is that expected? If you want to play around with a real world project that has this issue, check: https://github.com/epicweb-dev/rocket-rental

Copy link
Contributor Author

@pcattori pcattori Mar 10, 2023

Choose a reason for hiding this comment

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

Also, I've noticed that HMR can be quite slow (several seconds after save). Is that expected? If you want to play around with a real world project that has this issue, check: https://github.com/epicweb-dev/rocket-rental

Yes, planning on optimizing for perf soon. Though we are also limited by how quickly the app server can pick up changes. E.g. if using nodemon, how quickly can the app server be restarted. I know for Rocket Rental you are using tsx watch, so that might be a limiting factor.

That said, I think we should be able to make app server restart only blocking for HDR, and not for HMR (e.g. if we can serve the updated assets immediately from the dev server). For making HDR faster, server-side HMR (instead of server restart) would be needed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think my server is restarting. I'm doing the purge require cache approach instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think my server is restarting. I'm doing the purge require cache approach instead

Ah whoops. Saw tsx watch and made assumptions, but you are only watching for changes in ./index.js and not build/index.js. Make sense.

Yea so to summarize, we can optimize HMR speed directly, but have limitations on HDR speed depending on how fast your app server responds with an up-to-date build hash via __REMIX_ASSETS_MANIFEST endpoint. Should be able to at least speed up HMR part as part of stabilizing this feature.

Copy link
Member

Choose a reason for hiding this comment

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

That's great to hear! Thanks Pedro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants