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

fix(react): conditionally self-accept fast-refresh HMR #10239

Merged
merged 8 commits into from Oct 5, 2022

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Sep 25, 2022

Description

Fixes #9869
Fixes #3301
Fixes #4298
Fixes #6885
Fixes #7396

Based on top of #10244

This makes a change to the HMR code that's injected by plugin-react to check the exports of a module, and determine whether it is safe to treat as a fast-refresh boundary. If it is a safe fast-refresh boundary, we will perform fast-refresh. If not, we now call hot.invalidate() (relying on the changes in #10244) to propagate HMR up to parents. This is necessary because vite sees import.meta.hot.accept() even if it's inside a conditional, and treats the module as self-accepting. So without calling hot.invalidate(), the parent will never know to fast-refresh.

I've also added a test here for changing a react context file, and verified that it fails without the changes here, but passes with them.

Additional context

The other PR, #10244 should be reviewed first, since this one depends on it.

Thanks for help on this, @aleclarson!

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes vitejs/vite#123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@IanVS IanVS changed the title [Plugin-React] WIP: Conditionally self-accept HMR fix: Conditionally self-accept fast-refresh HMR [WIP] Sep 25, 2022
@aleclarson
Copy link
Member

aleclarson commented Sep 25, 2022

I pushed a commit here that implements the hot.invalidate API (a45385f), but it probably deserves its own PR. If anyone wants to push that forward, please do. I've no time currently.

Todo

  • Figure out how to avoid calling hot.invalidate on first page load, or have the dev server detect it as such and do nothing in response.

@IanVS IanVS force-pushed the issue/9869-fast-refresh-boundaries branch from a94c657 to 37dbbd0 Compare September 26, 2022 01:49
@IanVS IanVS changed the title fix: Conditionally self-accept fast-refresh HMR [WIP] fix: Conditionally self-accept fast-refresh HMR Sep 26, 2022
@IanVS IanVS force-pushed the issue/9869-fast-refresh-boundaries branch from 56f52e1 to 5d6b80e Compare September 26, 2022 13:51
@IanVS IanVS force-pushed the issue/9869-fast-refresh-boundaries branch from 5d6b80e to 535aa4f Compare September 28, 2022 21:33
@IanVS
Copy link
Contributor Author

IanVS commented Sep 28, 2022

@aleclarson I've rebased this onto main now that the hmr.invalidate() PR is in. Thanks again for all your help on this.

luthfib added a commit to zeus-health/ctw-component-library that referenced this pull request Oct 4, 2022
Moving the CTW Context and provider into separate files to avoid issues
when changing certain files it errors out and says context is not
defined. This is caused by a bug in vite which there is a current open
pr on.. vitejs/vite#10239, which should solve
this. Being that this bug is annoying enough in dev i am splitting
context and provider into separate files to avoid this bug.
melocule pushed a commit to zeus-health/ctw-component-library that referenced this pull request Oct 4, 2022
Moving the CTW Context and provider into separate files to avoid issues
when changing certain files it errors out and says context is not
defined. This is caused by a bug in vite which there is a current open
pr on.. vitejs/vite#10239, which should solve
this. Being that this bug is annoying enough in dev i am splitting
context and provider into separate files to avoid this bug.
luthfib pushed a commit to zeus-health/ctw-component-library that referenced this pull request Oct 4, 2022
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @zus-health/ctw-component-library@0.13.0

### Minor Changes

- 1086140: Condition History Panel now has a stacked UI list detailing
previous occurrences of different conditions. There is a preview summary
card followed by a detailed card for each of the different conditions
that have been recorded.
- fb5fd96: When user edits or adds a condition, practitioner id will now
be used to set the recorder field automatically. If no practitioner id
is found then recorder will be nulled else the current practitioner id
for the user will be used.

### Patch Changes

- 112d3df: Move ctw provider and context into separate files to avoid
bug outlined in <vitejs/vite#10239>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@sapphi-red sapphi-red changed the title fix: Conditionally self-accept fast-refresh HMR fix: conditionally self-accept fast-refresh HMR Oct 5, 2022
@sapphi-red sapphi-red changed the title fix: conditionally self-accept fast-refresh HMR fix(react): conditionally self-accept fast-refresh HMR Oct 5, 2022
@sapphi-red sapphi-red added the p4-important Violate documented behavior or significantly improves performance (priority) label Oct 5, 2022
@patak-dev patak-dev merged commit e976b06 into vitejs:main Oct 5, 2022
15 checks passed
@IanVS IanVS deleted the issue/9869-fast-refresh-boundaries branch October 5, 2022 12:04
@abouroubi
Copy link

Thanks for the great job ! I think this was a blocking issue for a lot of us React users.

There is a small issue with the CHANGELOG as this PR is not referenced there (even if it's int he commit history of release: v3.2.0-beta.1).
Regarding the number of people waiting for this fix, I think it would be a good idea to mention it.

@IanVS
Copy link
Contributor Author

IanVS commented Oct 10, 2022

Hi @abouroubi, this change was made to @vitejs/plugin-react, so the changelog is in that project: https://github.com/vitejs/vite/blob/plugin-react@2.2.0-beta.0/packages/plugin-react/CHANGELOG.md.

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