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

Fix #194 #195

Closed
wants to merge 3 commits into from
Closed

Fix #194 #195

wants to merge 3 commits into from

Conversation

alfonsogarciacaro
Copy link
Member

Ok, this is my new attempt to fix the FunctionComponent.Of API #188 #194. To make sure users are always passing the same render function to each FunctionComponent.Of I'm storing a stringified version of the function in the cache and compare it with newly passed render arguments. Seems to work but please someone give it a try to double check. I'm doing this only in Debug mode to avoid the overhead in production. I hope it's enough but let me know what you think.

This PR also updates npm dependencies to remove the security warnings (there's still one about yargs-parser when running npm audit that I couldn't solve).

@MangelMaxime
Copy link
Member

Looking at the code, I think it should work.

I guess we should try to release a beta version this time and ask people having the issue with the upgrade to comment if that helps them or not. Either by fixing the problem or helping troubleshoot the problem :)

@GBirkel
Copy link

GBirkel commented May 14, 2020

I think this fixes a rendering issue we’re having with our recent merge at Demetrix...

@alfonsogarciacaro
Copy link
Member Author

alfonsogarciacaro commented May 14, 2020

Hi @GBirkel! Please note that unfortunately this is not a "fix", just a way to fail fast and warn the user. If you're having problems maybe it's safer to go back to Fable.React 5.x.

@MangelMaxime Should we unlist the 6.x package if users seem to be having issues?

@MangelMaxime
Copy link
Member

MangelMaxime commented May 16, 2020

@alfonsogarciacaro I don't know in general I don't like the unlisting package but if that avoid problem perhaps we should.

Also, now it has been out for several days/weeks so perhaps it is too late?

Releasing this new fast failing would help people make the transition should be enough in theory no?

@forki
Copy link
Collaborator

forki commented May 18, 2020 via email

@MangelMaxime
Copy link
Member

@alfonsogarciacaro Should we release this new version?

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