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

Handle memo for function declarations / function assignments [publish] #45

Merged
merged 8 commits into from
Jul 21, 2024

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Jul 13, 2024

Fixes #44

cc @ArnaudBarre

The PR also slightly improves matching performance by replacing Array#includes w/ Set#has (where the previous one is O(n) while the latter one is O(1)).

@SukkaW SukkaW marked this pull request as ready for review July 13, 2024 07:01
@ArnaudBarre
Copy link
Owner

ArnaudBarre commented Jul 15, 2024

Well JS perf is not as simple. For a long time for small sets it was faster to use arrays than Set, because they have been in the engine for a long time and more optimized. This seems to not be true anymore on V8, and the opposite on JSC where array seems unoptimized. perf link

Sadly set creation is still not optimized enough compare to array creation, so sets should not be created when use only a few times: perf link

Given the current status of engines, I think your change is good expect for the creation of allowExportNamesSet, because for most cases it will be undefined and this doesn't seem to have a short path in v8: perf link

For the feature side, I think we don't need to check that deep, something that looks like export default memo(Identifier) should be allowed, if the identifier is not a react component it will just fail at runtime and we don't need to handle that

@SukkaW
Copy link
Contributor Author

SukkaW commented Jul 16, 2024

Given the current status of engines, I think your change is good expect for the creation of allowExportNamesSet, because for most cases it will be undefined

I can do const allowExportNamesSet = allowExportNames ? new Set(allowExportNames) : undefined.

For the feature side, I think we don't need to check that deep, something that looks like export default memo(Identifier) should be allowed, if the identifier is not a react component it will just fail at runtime and we don't need to handle that

Hmmm, that makes sense. Identifier will always be named since it is an Identifier after all.

@ArnaudBarre ArnaudBarre changed the title Handle memo for function declarations / function assignments Handle memo for function declarations / function assignments [publish] Jul 21, 2024
@ArnaudBarre ArnaudBarre merged commit 897a11d into ArnaudBarre:main Jul 21, 2024
@ArnaudBarre
Copy link
Owner

Thanks for the PR, this is published in v0.4.9!

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.

Possible false-positive with memo default export
2 participants