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

JSX Spread Syntax requires ES6 Object.assign #2417

Closed
dandelany opened this issue Oct 28, 2014 · 8 comments
Closed

JSX Spread Syntax requires ES6 Object.assign #2417

dandelany opened this issue Oct 28, 2014 · 8 comments

Comments

@dandelany
Copy link

The new JSX spread syntax introduced in 11.2 and described here:

https://gist.github.com/sebmarkbage/07bbe37bc42b6d4aef81

is very nice, but when I compile JSX that uses this feature, I see it is implemented with Object.assign. Since this is a proposed ES6 feature that is only supported in Firefox 34, using JSX spread syntax necessitates using a polyfill for Object.assign. It would be great if this were not the case, and seems like it should be possible to implement this behavior without Object.assign.

@jhiswin
Copy link
Contributor

jhiswin commented Oct 28, 2014

I actually like the idea of using Object.assign, and encouraging the use of the ES6 standard. However, it seems counterproductive/redundant to implement Object.assign for internal use only, and exposing it as React.__spread, especially when Object.assign is so commonly used.

Same with a couple other internal APIs, such as listen(), which could be exposed so it doesn't have to be implemented twice.

@dexbol
Copy link

dexbol commented Oct 29, 2014

I think it's too radical! At least the doc should explain something like "ES6 only , you can use transferPropTo() function for compatibility".

Due to this case , I feel React is not credible.

There are popular words in Chinese "步子大了容易扯到蛋" that means “ If your step is too big, your ball will break up”

@sebmarkbage
Copy link
Collaborator

@dandelany This feature was never announced in 0.11.2 because it wasn't quite done yet. In 0.12, the spread attributes in JSX do not require Object.assign. It calls into React._spread for now.

The original intention was to require an Object.assign polyfill for all of React Core just like we do for ES5 features. This is the direction we're heading. Checkout this presentation for rationale: http://2014.jsconf.eu/speakers/sebastian-markbage-minimal-api-surface-area-learning-patterns-instead-of-frameworks.html

Both our --harmony transpiler and React will likely take on dependencies on ES6 polyfills in the future. Especially for ES7 features.

Unfortunately, the wider JS community's polyfilling infrastructure is pretty poor. So we eventually decided against taking on the dependency in React Core for now.

@dexbol The main docs was updated with alternative patterns if you don't want to follow along with the progress of standards. http://facebook.github.io/react/docs/transferring-props.html#transferring-with-underscore

@sebmarkbage
Copy link
Collaborator

Closing out since this was already addressed in 0.12.

@dandelany
Copy link
Author

@sebmarkbage Thanks for the explanation, and apologies for the confusion. I had installed 0.12 but was accidentally still testing this on a project that used 0.11.2.

Great presentation, and I definitely agree with you re: reducing the API surface area and bringing abstractions under control. I, too, though, am a bit worried about how polyfilling will work. Seems like you have a few options, none of which are great:

  1. Bundle polyfills internally in React, under eg. the React namespace as modules. Bad because of problems mentioned here and the fact that you can't refer to them via normal syntax

  2. Bundle polyfills with React but do so by overwriting globals eg. Object.assign. Seems weird/bad practice to touch globals, and the user could already have a polyfill in place which you're "re-polyfilling". But this is still maybe the best option.

  3. Require users to have their own polyfills in place for React to work properly - allows people to use the polyfills they want but significantly increases the barrier to entry to getting React working for a new dev.

I'm curious what React's planned approach is... definitely not an easy problem to solve.

@rymohr
Copy link

rymohr commented Nov 20, 2014

I'm still seeing this behavior with 0.12.0 (through react-rails). This should work yeah?

render: function() {
  return <MyComponent {...this.props} />;
}

// compiles to
render: function() {
  return MyComponent(Object.assign({}, this.props));
}

@sebmarkbage
Copy link
Collaborator

That seems like react-rails might be still using the old compiler. That's not the 0.12.0 behavior. Perhaps you're using the 0.12.0 runtime with the 0.11 compiler.

@rymohr
Copy link

rymohr commented Nov 20, 2014

Thanks @sebmarkbage. It was actually an old version of reactify that was being called from browserify-rails that was the culprit.

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

No branches or pull requests

5 participants