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.

Sign up for GitHub

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

Jump to bottom

[plugin-react-refresh] Solve the problem with MobX observer() HOC #3015

Merged
merged 3 commits into from
Mar 30, 2021
Merged

[plugin-react-refresh] Solve the problem with MobX observer() HOC #3015

merged 3 commits into from
Mar 30, 2021

Conversation

yuhongda
Copy link
Contributor

@yuhongda yuhongda commented Mar 27, 2021 •
edited
Loading

Changes

It's all start with a bug of react-refresh. react-refresh doesn't work with MobX observer HOC. so @gaearon makes a fix on it. But the fix is not work with snowpack. After I checked all the babel plugins and transformers, I found out it's problem with plugin-react-refresh.
The story: facebook/react#20417 (comment)

The function transformHtml() in plugin-react-refresh insert the react-refresh code using contents.replace(), so the $ will have meaning in the replaced content. The code of node_modules/react-refresh/cjs/react-refresh-runtime.development.js has these lines:

if (typeof type === 'object' && type !== null) {
      switch (getProperty(type, '$$typeof')) {

After replace, become:

if (typeof type === 'object' && type !== null) {
  switch (getProperty(type, '$typeof')) { // NOTE: ONLY ONE $

That's the reason why @gaearon's fix not working. So I make a fix just like below:

image

Testing

For testing this issue, I create a sample project to test it. Here is the repo: https://github.com/yuhongda/repro-react-issues-20417

  • NOTE: And, I update the snapshot in plugin.test.js.snap. (I replace all the $typeof to $$typeof, so it will compatible with new version of react-refresh)

Docs

bug fix only

@vercel
Copy link

vercel bot commented Mar 27, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/EXkeGVvpZExJGXdGYcovFGfWzbcL
✅ Preview: https://snowpack-git-fork-yuhongda-fix-plugin-react-refres-d20c0f.vercel.app

@gaearon
Copy link

gaearon commented Mar 27, 2021

nice find

@yuhongda yuhongda mentioned this pull request Mar 27, 2021
Closed
@winderica winderica mentioned this pull request Mar 27, 2021
Merged
@FredKSchott
Copy link
Owner

LGTM! Appreciate the PR description as well, it was a fun read :)

I can see why snapshot updating didn't work (you need to run yarn test:dev -u) I'll get this merged and then update those for you on main. Thanks for the contribution!

@FredKSchott FredKSchott merged commit 6e4bb7f into FredKSchott:main Mar 30, 2021
@FredKSchott
Copy link
Owner

Sorry, I had to revert: this caused a bad HMR injection in the basic create-snowpack-app for React. To reproduce:

  1. create a new app-template-react project
  2. snowpack dev
  3. See the error: Uncaught ReferenceError: $RefreshSig$ is not defined

This is due to a commented out <body> tag in the HTML.

FredKSchott added a commit that referenced this pull request Mar 31, 2021
@FredKSchott
Revert "[plugin-react-refresh] Solve the problem with MobX observer()…
a82cb6b 
… HOC (#3015)"

This reverts commit 6e4bb7f.
@yuhongda
Copy link
Contributor Author

Sorry, I had to revert: this caused a bad HMR injection in the basic create-snowpack-app for React. To reproduce:

  1. create a new app-template-react project
  2. snowpack dev
  3. See the error: Uncaught ReferenceError: $RefreshSig$ is not defined

This is due to a commented out <body> tag in the HTML.

Okay, I got it. I'll push a fix version.

@yuhongda yuhongda mentioned this pull request Mar 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuhongda @gaearon @FredKSchott