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.

Sign up for GitHub

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

Jump to bottom

Doesn’t work with React.createFactory() #277

Closed
gaearon opened this issue May 2, 2016 · 6 comments
Closed

Doesn’t work with React.createFactory() #277

gaearon opened this issue May 2, 2016 · 6 comments
Labels
bug

Comments

@gaearon
Copy link
Owner

gaearon commented May 2, 2016 •
edited
Loading

The issue is explained here: #276 (comment). It seems like we’d need to monkeypatch React.createFactory because it bypasses React.createElement and uses the internal ReactElement module directly.
@gaearon gaearon added the bug label May 2, 2016
@gaearon gaearon added this to the v3.0 milestone May 2, 2016
This was referenced May 2, 2016
Closed
Closed
@nfcampos
Copy link
Collaborator

nfcampos commented May 2, 2016 •
edited
Loading

theres no need to patch React.DOM.* beause we only care about composite components right?

@gaearon
Copy link
Owner Author

gaearon commented May 2, 2016

Haha, excellent point! I’m being silly.

@gaearon gaearon changed the title Doesn’t work with React.createFactory() or React.DOM.* factories Doesn’t work with React.createFactory() May 2, 2016
@nfcampos
Copy link
Collaborator

nfcampos commented May 2, 2016

:) also, how about patching only the internal react/lib/ReactElement? surely they all import createElement from that one no?

@gaearon
Copy link
Owner Author

gaearon commented May 2, 2016

We could do this. It wouldn’t work in the future though when React ships flat bundles. Also doesn’t work for people who use precompiled builds of React. (Not sure we really want to support it, but right now we do.)

@nfcampos
Copy link
Collaborator

nfcampos commented May 2, 2016 •
edited
Loading

I've added the failing test case for this on #278

@nfcampos nfcampos mentioned this issue May 4, 2016
Merged
@gaearon
Copy link
Owner Author

gaearon commented May 5, 2016

Fixed in 3.0.0-beta.2.

@gaearon gaearon closed this as completed May 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nfcampos @gaearon