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

chore!: drop ast check for refresh boundary #43

Merged
merged 1 commit into from Dec 5, 2022

Conversation

ArnaudBarre
Copy link
Member

The runtime only check is closer to what other implementation are doing including the vite SWC one.

This is mostly for simplification and consistency between plugins. It will also have the benefit of allowing people to know when the fast refresh was skipped when I will port the invalidate message from the SWC plugin.

cc @IanVS

@IanVS
Copy link

IanVS commented Dec 5, 2022

I think I'm okay with this, but @aleclarson suggested to keep the AST check in place when I was adding the runtime check. So maybe he has some thoughts about this.

Ref: https://discord.com/channels/804011606160703521/1018203348060618782/1019332955551834162

@aleclarson
Copy link
Member

aleclarson commented Dec 5, 2022

maybe he has some thoughts about this.

The AST check avoids an unnecessary runtime check and code transformation in many cases. I haven't measured the performance impact of removing the AST check, so I can't help you decide whether merging this is the best option or not.

@patak-dev
Copy link
Member

I think it is a good idea to align with other implementations. But let's remove this once we know the perf implications.

@ArnaudBarre
Copy link
Member Author

I can do some perf checks but I disagree with:

The AST check avoids an unnecessary runtime check and code transformation in many cases

The ast check is always done after code transformation, so you can't skip transformation. It's done only when babel already added some "Refrehreg" code, so in >>90% of the cases for codebase that respect rules (and you should otherwise HMR is a nightmare), you will run both AST and runtime checks that both says everything is valid.

@patak-dev patak-dev merged commit e43bd76 into main Dec 5, 2022
@patak-dev patak-dev deleted the drop-ast-boundary-check branch December 5, 2022 14:32
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

4 participants