inlineElements optimization breaks on older browsers; discussion about Symbols and React.elementFromObject() API

[STRML]

See this babel/babel#2517 and the associated discussion around the PR babel/babel#2518, which I don't expect to be merged due to loader issues.

To recap:

The inlineElements optimization requires brittle knowledge of internal React values, namely, $$typeof. This breaks on older browsers unless the developer globally polyfills Symbol, because Symbol will be polyfilled automatically by Babel in the user's code, but will not be polyfilled in the React library. This causes ReactElement.isValidElement to fail as Symbol.for('react.element') !== 0xeac7.

Worse, this bug only occurs in older browsers that don't implement Symbol, meaning that many devs won't catch it right away as it will work fine in FF, Chrome, and (latest) Safari.

This is a hard issue to fix without globally polyfilling Symbol or giving up on the use of Symbol for $$typeof. Babel could automatically this as part of enabling the optimisation, but @loganfsmyth had a better idea - how about a React.elementFromObject() API?

This function would be nothing more than:

React.elementFromObject = function(obj) {
  invariant(obj && typeof obj === 'object', "Supply an object to React.elementFromObject.");
  obj.$$typeof = REACT_ELEMENT_TYPE;
  return obj;
}

This ensures that the REACT_ELEMENT_TYPE we are using is equal to the one used in ReactElement.isValidElement. It shouldn't be necessary to do any validation in elementFromObject because it will be caught by isValidElement later on.

Thoughts?

[sebmck]

The purpose of inlining the elements is to get rid of function calls so I don't think adding a React.elementFromObject call is a good idea since it defeats the point.

[STRML]

I thought so too at first. But the really expensive part of React.createElement() is not the function call itself, but the props and children copying and the associated array and object allocations. Currently, when using external helpers, there's a function call anyway to get REACT_ELEMENT_TYPE.

I think our discussion re: using React.REACT_ELEMENT_TYPE will work just fine, but it would be nice for React to be able to fully abstract over this so we don't need to know what $$typeof is or what magicnum/Symbol it uses.

[gaearon]

I feel that adding a new “blessed” top-level API to work around a bundling / polyfill issue is a bit of an overkill. (By “blessed” I mean something like elementFromObject.)

[STRML]

Perhaps, and it might lead to some confusion in the future - but it would be nice for other (non-babel) tooling in the future, like cljs, that wants to benefit from using raw objects.

[loganfsmyth]

@gaearon I'd argue that it doesn't seem like a bundling issue in the long run. The core issue is that React should not accept random objects as components. What better way to do that than to expose an API that allows that? The current functionality is an API, but it relys on the global symbol registry and it's not clear (to me at least) why that approach was taken, or what makes that better than exposing a function or constant from React directly instead. That would also mean you could avoid relying on the random integer constant, and actually have the constant be a normal object that could be safely compared with ===, and it avoids relying on possibly-polyfilled Symbols alltogether.

[zpao]

cc @sebmarkbage

[sebmarkbage]

The Symbol solution ensures that multiple npm packages or instances of React can coordinate the same identity seamlessly. Even across realms. None of the proposals here address that concern. (It also makes us compatible with a theoretical future Value Types proposals.)

If React can coordinate with multiple versions of itself, then Babel can as well.

One possible solution would be that Babel depends on a module that exposes this or somehow opts out of inlining the polyfill so that it gets the same fallback as React itself.

[loganfsmyth]

I'm totally pro-symbol, to be clear, I was mostly surprised to see that the symbol registry was the actual using facing UI, rather than having React internally pull a symbol from the the registry and expose it as a constant.

[STRML]

I left a comment on the original PR re: the magicNum - why not just use Infinity, which is portable across realms, doesn't require a polyfill, fixes the Babel problem, and fixes the XSS problem.

[STRML]

This problem is still present and will cause unexpected breakage in older browsers in production builds only.

Since profiling has shown that the function call is actually not the bottleneck, I think it makes sense to put this back on the table as a blessed API (if even somewhat undocumented) as not to break apps in production. It would be great for interop with other optimizing runtimes as a fast path alternative to React.createElement() with fewer validations.

Alternatively, could the ReactElement constructor (not ReactElement.createElement) simply be exposed?

Babel helpers have this interesting desire to remain "pure", but I don't consider relying on a Symbol polyfill (and failing interop between a polyfill and native) to be pure. This seems to be the only long-term solution that will fix this for good.

[sebmarkbage]

How does this work with Symbol.iterator which is also a shared symbol that need to be handled the same way between for...of and library code?

[STRML]

In that case, Symbol would be polyfilled in the library code (it would have to be if you used Symbol.iterator), and both polyfills would fall back to global.Symbol if present.

The crux of the problem here is that React has a fallback for when Symbol is not present, and it's not to polyfill it - it's to use a magicnum. So it's possible that user code uses a polyfill (say via Babel), but the library does not and thus uses the magicnum, and the magicnum !== the polyfill.

I'm thinking the way to fix this is not to fix babel-plugin-transform-react-inline-elements, but to make its existence unnecessary. Originally, it was thought that replacing the function call with an inline object would be faster, but it turns out that's not true and the transform now uses the _jsx() function.

If there's a function that can create React elements faster in production than React.createElement() does, shouldn't that just be the actual implementation of React.createElement() in production mode? I'm aware there may be some loss of context in error conditions, but errors are generally thrown away in production anyway - or this could be configured.

[sebmarkbage]

The inlining is only part of the story. The point is to decouple this from the React package itself and not needing that dependency.

I think you're missing the point though. The point is to explore a generic solution around nominal branding. E.g. for Polyfilling pattern matching. Not just for React's particular value type but any number of similar data structures. That's why we wouldn't use Infinity because only one library can use that trick and then it is used forever.

There is also a larger issue about how polyfills should work across these boundaries. I suspect that this partial polyfilling solution is the thing that is broken and will break down in more ways than this particular case. Others will have similar problems. Symbol.for is a feature that library authors should be able to use. Therefore the transpile/polyfills story should be fixed.

It is not obvious to me why React's fallback is different from other feature tests that is standard practice. E.g. we also use var assign = Object.assign || function() { /* internal polyfill */ }. It is obviously not the problem with feature testing per say. It is that there is global identity required so it needs to be the same polyfill.

What would be the best way to polyfill Symbol.iterator in library code? E.g. both React and immutable-js check for the string "@@iterator" as a possible property name. Why doesn't this cause the same problem? https://github.com/facebook/immutable-js/blob/d94141223d56ac5bd6f541b06baa3161c4edd01b/dist/immutable.js#L185

A similar problem would occur with anything that requires global state. The spec has intentionally tried to avoid global state but for example the Zones proposal would have a similar requirement. I suppose that when you have global state, you really need to have a shared object reference and a global polyfill is the only solution. So, maybe global polyfills should be best practice?

[STRML]

This is the purpose of @@toStringTag, isn't it? Rather than React.Element defining a '$$typeof', it should define React.Element[Symbol.toStringTag] = 'ReactElement'.

In non-ES6 environments, we can't prevent XSS in a way that matters and is robust. There is no solution (aside from using types that are not JSON-serializable, but of course this is one-and-done if a library uses it) that actually works. We could set $$typeof: Infinity but inline-elements would then have to both set $$typeof and @@toStringTag.

I think the magicnum should just be ditched entirely, which would fix the bug. ReactElement.isValidElement should check typeof Symbol === 'function and Symbol.toStringTag, if it exists, returns element[Symbol.toStringTag] === 'ReactElement', otherwise just return true.

Re: Object.assign, it's not the same thing. Object.assign polyfills and "the real thing" have the same result. By definition, a Symbol polyfill and an actual Symbol cannot have reference equality, and reference equality is the primary use of a Symbol.

The same bug also affects Symbol.iterator and has not been resolved. In fact, there were previously problems simply with multiple instances of the polyfill returning different symbols!

Reproduction: Say I pass an iterable into React, in an environment without native Symbol, where userland code has the polyfill but libraries do not:

• That iterable has the property I[Symbol.iterator], where Symbol.iterator is a polyfill
• React checks if the global Symbol exists; it doesn't, so it checks for I['@@iterator']
• That also doesn't exist so it assumes the collection is not iterable.
• An error is thrown as you can't iterate a plain object.

I've run this to verify and it indeed fails in the case where the runtime is missing support, such as in Node 0.10, but succeeds in newer runtimes like 5.5.

In the specific case of React, there are a few actions that should be taken:

1. Remove '$$typeof' in the next release and simply return true from ReactElement.isValidElement() if there is no global Symbol and the input is an object. There will be no real security here in ES5 environments without a global polyfill unless we're willing to claim Infinity, and even then it doesn't save us from structured cloning vectors.
2. Add a check for object[Symbol.toStringTag] === 'ReactElement'.
3. Evaluate whether the inline-elements transform should even exist. Since a function call can still be fast, why can't production React make the same optimizations?
   1. If it's decided that the optimization should still exist, update Babel to set Symbol.toStringTag after checking the global.

[sebmarkbage]

I'm hoping that we will be able to find a way to share objects across the postMessage API. Between workers or even cross-origin. Part of that would be to allow symbols to transfer between these workers.

If the toStringTag is shared across workers then this is object[Symbol.toStringTag] === 'ReactElement' spoofable. However, the same is true if Symbol.for shares identity so maybe neither should share identity. There is a lot more research needed for this space.

I admit that this is a long shot and many pieces need to fit together but something to consider.

In the meantime, the way I read this is there are three variables:

1. Using global polyfills everywhere or only local polyfills.
2. Using Symbol.for('react.element') as a branding strategy.
3. Using Symbol.iterator to detect iterability.

If you use local polyfills, then both 2 and 3 breaks and your code is broken. So we have to fix something.

a) If we fix only 2, then 3 still breaks and your code is broken.
b) If we fix both 2 and 3, by changing how Symbol.iterator works then your code works.
c) If we avoid the use of local polyfills and ensure that everyone uses global polyfills, then both 2 and 3 work fine.

Option A doesn't fix anything so it is not very helpful.

Option B won't work unless we can change how all libraries do this feature detection of Symbol.iterator, or if we can change the specification. Neither seems realistic.

Option C seems the most plausible to me. If we manage to fix Option C there is no point in changing how React works.

Are there any other options?

[STRML]

You mentioned before that it was intentional that these objects would not work across postMessage boundaries, and that they should instead be explicitly put through a constructor that will tag them so there is no chance of raw data coming across a boundary being interpreted as an element.

[sebmarkbage]

The idea is that they would not work across postMessage by default - unless you explicitly provided a way to share the symbol.

[pastelsky]

Any progress of this? Is there a workaround for this till the issue is fixed that doesn't involve adding babel-polyfill to the entry point? I tried using es6-symbol/implement polyfill, but that didn't work.

[STRML]

@shubhamsizzles For now I put this in my browser entrypoint so all libs are using the same Symbol.

global.Symbol = require('core-js/es6/symbol'); // Fix react.inlineElements

[gaearon]

I’ll close this as stale.

[STRML]

Unfortunately this is still an issue on old browsers unless you globally polyfill Symbol. Have there been any strategies discussed internally that didn't make it here?

[gaearon]

No, there haven't. My impression is we just don't support this use case. You could either use a global polyfill or not enable the transform.

[STRML]

I think many tool authors would be very happy with a React.elementFromObject(), even an unstable_ one, or SECRET_LUL_WUT_R_U_DOING version so long as it gets the job done. Otherwise it's just done implicitly through Symbol.for() and simply not as reliable. Worse still, it works fine in modern dev browsers - this will break in a hard-to-find way for legacy users only. Hope your team will consider.

[gaearon]

Meh. I don’t see us fixing this. We rely on Symbol.for() in more places. Even if we provided elementFromObject() those cases would still be broken. So arguably it’s even worse as it would further narrow down the cases where it breaks (but not completely fix it).

Our suggestion is: if you use a Symbol polyfill, it is on you to ensure it’s the same one used globally throughout your code. We’re happy to accept PRs emphasizing this in the documentation.

[STRML]

This action would still be relevant:

Simply return true from ReactElement.isValidElement() if there is no global Symbol and the input is an object with a $$typeof property. There will be no real security here in ES5 environments without a global polyfill unless we're willing to claim Infinity, and even then it doesn't save us from structured cloning vectors.

The magicnum can be trivially bypassed via useragent detection from a determined adversary targeting XSS into React. It causes more problems than it solves.

[gaearon]

input is an object with a $$typeof property

That would technically not be right, would it? We use other $$typeof values too, not just the one for the elements.

[STRML]

Perhaps another way we can duck-type it? The point is, XSS detection without Symbol or a polyfill is broken, and we shouldn't fail-closed on a mismatch.

[gaearon]

My point is that it's wrong to include inline-elements polyfill but not polyfill Symbol globally. Is that not the case?

[STRML]

Only because of ES5 browsers, and only because of the isValidElement() check. It will work completely fine in any modern browser (which practically all devs are using for work), so it will go unnoticed unless we do something drastic like require the global shim (with an opt-out option in the transform) or create a very loud warning. I don't love either mitigation when isValidElement() is not doing particularly useful work to begin with on ES5 browsers.

[gaearon]

We use Symbol.for in more places, and not just for isValidElement alone. So those would still be broken. I think you’re looking at the issue from a point of view of a particular transform that depends on the logic. Whereas I’m looking at it from the perspective of being able to use Symbol.for() in the codebase in general.

it will go unnoticed unless we do something drastic like require the global shim (with an opt-out option in the transform) or create a very loud warning

If you mean the Babel plugin in particular, IMO it is expected that people read the README of the plugin they’re adding. If they’re not trusted to do that then how can they be trusted to only enable it in production. (Or similarly, enable source plugin only in development.)