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

FunctionComponent.Of extend func cache key with 'Props type name #242

Merged

Conversation

DunetsNM
Copy link
Contributor

User code that invokes FunctionComponent.Of can also be in generic context which makes caching by source FileName+LineNumber unsuitable: one function gets cached and resolved at runtime for different runtime types which results in runtime exceptions.

I propose to add 'Props type name to the cache key to fix that.

You still can write code were wrong function will be picked up at runtime for the same 'Props argument type (e.g. if different render func values are passed from the outside), but it's less likely to happen.

Ultimate solution would be to expose the fact that function is prepared and cached to the users and let them to specify their own unique cache key - or piggyback on existing displayName, which is also not ideal.

@DunetsNM
Copy link
Contributor Author

this fixes #241

@DunetsNM DunetsNM force-pushed the dev/fix-functioncomponent-caching branch from 9c78080 to 2f39899 Compare May 17, 2024 07:10
@DunetsNM DunetsNM force-pushed the dev/fix-functioncomponent-caching branch from 2f39899 to e93ecf9 Compare May 17, 2024 07:12
@MangelMaxime
Copy link
Member

To avoid impacting too much the bundle size, we could extract the method body into a function and pass it the typeof<'Props>.FullName + all the required information as an argument.

What do you think?

@DunetsNM
Copy link
Contributor Author

@MangelMaxime I moved the fat part to Cache.GetOrAdd to avoid creating more public/inlineable impl functions

@MangelMaxime
Copy link
Member

Thank you

@MangelMaxime MangelMaxime merged commit a35a41f into fable-compiler:master May 24, 2024
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

2 participants