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

Could we make it less painful to inline ReactElement objects? #3285

Closed
ariabuckles opened this issue Feb 28, 2015 · 9 comments
Closed

Could we make it less painful to inline ReactElement objects? #3285

ariabuckles opened this issue Feb 28, 2015 · 9 comments

Comments

@ariabuckles
Copy link
Contributor

Hi!

@spicyj said I should open an issue here. Disclaimer: I'm trying to do sort of sketchy things, and I know this isn't supported, but I think it would be nice.

I'm writing a library that I want to support react output optionally. To do this, right now I'm trying to inline all my ReactElements, such as

    return {
        type: 'hr',
        key: state.key,
        _isReactElement: true
    };

instead of

    React.createElement('hr', {
        key: state.key
    })

This is almost completely functional, except for the _store validation in react dev mode. This is because right now these literals don't have a _store key with .validated or .originalProps, which are assumed by the validators in React DEV. I can work around this, but to do so without duplicating props will have to write a wrapper function to create the element, which is a little unideal and slower (and adds unnecessary keys in non-dev mode).

Would it be reasonable to short circuit some of these checks so that they only happen if _store is actually present on the ReactElement? If so, I'd be happy to submit a pull request (no rush for react 0.13 though).

Curious as to your thoughts. Thanks!
Aria

@sophiebits
Copy link
Collaborator

(To be clear, this would be about changing checks like if (element._store.validated || element.key != null) in ReactElementValidator to not assume that element._store exists.)

@sebmarkbage This one's for you. :)

@syranide
Copy link
Contributor

syranide commented Mar 1, 2015

#3228 sounds very much related (but again, lack of warnings is an issue)

@sebmarkbage
Copy link
Collaborator

Note that createElement is responsible for both key AND propTypes warnings as of 0.13.

@ariabuckles The way I've been thinking about this problem is that this should definitely be possible to inline objects in your code. #3228 would only enable this as a production mode compiler feature. However, it should ONLY be ok in the case that you have some other way of providing valid warnings.

I don't think that simply opt-ing out of these warnings completely is a viable strategy. You might think that you won't mess up propTypes or keys as you're manually inlining these. Even if you don't, you might be accepting children or props from the outside that does need to be validated.

You might think that these warnings are coddling developers and that we should just allow you to opt out. However, this undermines the stability of shared components in a larger community - which ultimately will cause people to blame React.

One idea was to use a static type system like Flow. If you use a static type system, your files can be statically verified, and therefore you shouldn't need to validate them dynamically, even in development mode. I'm not sure how to accommodate this and transfer the verification flag. Perhaps using a special transform which adds the _store.

Can you explain a bit more about your use case (even if it is sketchy)? It might help us come up with ideas for how to solve your special case without losing warnings for the common case.

@ariabuckles
Copy link
Contributor Author

Hi! Thanks for your response, @sebmarkbage.

I'm making https://github.com/khan/simple-markdown render to either react or html, and am figuring out how best to let html users not be messed up by a require("react") statement. There are a couple approaches I can take there, and not all of them require inlining elements, but inlining elements is promising because it completely removes the dependency on actually having React referenced by simple-markdown. Like you mention, it also means you can avoid the function call overhead in production, which given how many DOM nodes markdown often outputs, could be a win.

I'm not trying to avoid the warnings. I like react's warnings, and at KA we currently have most of them turned into alerts :). I'm mostly trying to avoid the dependency on React. I could solve this by dependency injecting it, which works but provides a slightly worse API for clients, and in this specific case I don't need refs and don't inject any children.

Unfortunately I am currently avoiding the warnings in my implementation, but (a) I would prefer not to in dev, although doing so is trickier, and (b) this specific case is a bunch of react elements I've hardcoded myself that don't have refs or external children, so I'm not too concerned about those warnings. key warnings would be nice, but at the same time the keys i'm using are pretty much the worst possible keys, and keyless elements wouldn't actually perform any worse.

Right now I've got a function for adding the _store prop to cheat validation ( https://github.com/Khan/simple-markdown/blob/react/simple-markdown.js#L212 ), but ideally could make it so that this function does validation in dev mode and skips it in production.

I'd probably not have said anything ordinarily, but Ben said I should.

@sebmarkbage
Copy link
Collaborator

It is definitely a goal to decouple the ReactElements from React itself. The idea is that a transpiler can have its own React.createElement implementation so that you don't need to have React in scope to generate these elements. Any such implementation would need to have both DEV and production mode I guess.

Another possibility is that we could start using WeakMap for _store as the element gets passed through.

@STRML
Copy link
Contributor

STRML commented Sep 11, 2015

This _store check in ReactElementValidator is causing exceptions when using babel's new inlineElements optimisation.

@zpao
Copy link
Member

zpao commented Sep 11, 2015

@STRML IIRC that is only seen when using the inline elements transform and the dev build of React. cc @spicyj

@STRML
Copy link
Contributor

STRML commented Sep 11, 2015

Yeah, this is in 0.14.0-rc1.

@sophiebits sophiebits added this to the 0.14 milestone Sep 11, 2015
@sophiebits
Copy link
Collaborator

I had meant to fix that check but I guess I missed it. But yes, you should use the prod build of React when using the inlined elements.

sophiebits added a commit to sophiebits/react that referenced this issue Sep 25, 2015
Seems better to fail gracefully, especially now that we support inlining. If people do this by accident we can figure out how to add a helpful warning instead.

Fixes facebook#3285.
sophiebits added a commit to sophiebits/react that referenced this issue Sep 25, 2015
Seems better to fail gracefully, especially now that we support inlining. If people do this by accident we can figure out how to add a helpful warning instead.

Fixes facebook#3285.
sophiebits added a commit that referenced this issue Sep 25, 2015
Don't blow up on missing _store in element validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants