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

Implement hoist constant JSX #31

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Jul 24, 2023

Still adding test cases.

@lxsmnsyc
Copy link
Owner

This is like the Babel plugin, right? The only concern here is that different JSX runtimes may treat these differently, hence why I didn't do this. Another thing is we have the auto-memoized JSX. Hoisted JSX doesn't really offer any wins besides maybe saving creation time that's just probably a minor perf improvement. But, like I mentioned, I'm probably looking at what violations would this affect.

@SukkaW SukkaW force-pushed the hoist-constant-jsx branch 2 times, most recently from ae617af to 080d1ab Compare July 24, 2023 08:08
@SukkaW SukkaW force-pushed the hoist-constant-jsx branch from 080d1ab to c19f1f6 Compare July 24, 2023 08:15
@SukkaW
Copy link
Contributor Author

SukkaW commented Jul 24, 2023

Another thing is we have the auto-memoized JSX. Hoisted JSX doesn't really offer any wins besides maybe saving creation time that's just probably a minor perf improvement.

The hoisted JSX can reduce the unnecessary branch call inside the component and reduce the output code size, by making hoisted JSX bailing out if (_condition) and (_euqal) ? :.

As for downsides, a potential downside would be the hoisted JSX expressions are not stored inside React/Preact's component life cycle and thus will not be thrown away when the component unmounts.

The only concern here is that different JSX runtimes may treat these differently, hence why I didn't do this

Maybe we can make it an option, and react and preact preset will have this option enabled.

@SukkaW SukkaW marked this pull request as ready for review July 24, 2023 10:11
@SukkaW SukkaW force-pushed the hoist-constant-jsx branch from 6068e69 to 67b51c9 Compare August 5, 2023 09:59
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.

2 participants