Skip to content

chore!: drop ast check for refresh boundary#43

Merged
patak-cat merged 1 commit intomainfrom
drop-ast-boundary-check
Dec 5, 2022
Merged

chore!: drop ast check for refresh boundary#43
patak-cat merged 1 commit intomainfrom
drop-ast-boundary-check

Conversation

@ArnaudBarre
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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-cat
Copy link
Copy Markdown
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
Copy Markdown
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-cat patak-cat merged commit e43bd76 into main Dec 5, 2022
@patak-cat patak-cat 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.

4 participants