Skip to content

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
Contributor

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.

5 participants