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

Remove pojo #15

Closed
wants to merge 1 commit into from
Closed

Conversation

davidtme
Copy link

@davidtme davidtme commented Dec 1, 2016

@alfonsogarciacaro
Copy link
Member

Hmm, this may require some investigation. According to the issue that generated the PR you linked they're removing the warning because of false positives, not because they don't care if it's a plain object or not.

This is an important distinction because React may remove the prototype methods of the object (I guess @et1975 had some trouble with that) and then things may fail at runtime (like equality comparison with .Equals).

@et1975
Copy link
Member

et1975 commented Dec 1, 2016

elmish no longer passes any user models to React as-is, they are always wrapped in a record, which is never treated as a valid F# object from then on. React shallow-mangles those records, but that's fine. For fable-react it might be a good idea to keep the attr, but I can't think of a specific reason - a callback with a previously supplied argument?

@alfonsogarciacaro
Copy link
Member

It's common in React to pass functions in the props object, and if this functions are in the prototype I'm afraid they may be removed :/

@davidtme
Copy link
Author

davidtme commented Dec 1, 2016

I've checked and react does keep the functions .Equals etc but it does mangle it so instanceof no longer works. It only seems to mangle the root prop so maybe I could wrap my record up?

@alfonsogarciacaro
Copy link
Member

The helpers in fable-react are a very thin layer on top of React. In a sense, Pojo forces you to wrap your record. More high-level libraries like fable-elmish can do this automatically for you. I'd rather keep the Pojo attribute for now, because it makes the user aware of the semantics and it plays very nicely with React developer tools.

@davidtme
Copy link
Author

davidtme commented Dec 2, 2016

Thanks @et1975 and @alfonsogarciacaro this has really been helpful. It's not safe to remove the 'Pojo' attribute.

@davidtme davidtme closed this Dec 2, 2016
2sComplement pushed a commit to 2sComplement/fable-react that referenced this pull request Jul 6, 2017
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.

3 participants