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

Add back React.__spread and make it warn #6444

Merged
merged 2 commits into from
Apr 8, 2016
Merged

Conversation

zpao
Copy link
Member

@zpao zpao commented Apr 7, 2016

This API was removed because it was undocumented and existed solely for our JSXTransformer/react-tools, which haven't been supported for a year. However it turns out TypeScript is still using this API in their TSX compilation and it kind of sucks to break people who want to upgrade React but not the rest of their toolchain.

Note: this doesn't quite (read at all) work with the Object.assign -> object-assign transform I wrote so would need to fix that too.

@sebmarkbage
Copy link
Collaborator

Can we make it say something about TypeScript and JSTransformer specifically so people know how to upgrade?

warning(
warned,
'React.__spread is deprecated and should not be used. Use ' +
'Object.assign directly or another helper function with similar semantics'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add trailing period

@KyleAMathews
Copy link
Contributor

Coffee-React uses it as well fwiw https://github.com/jsdf/coffee-react#spread-attributes

'Object.assign directly or another helper function with similar semantics'
);
warned = true;
return Object.assign.apply(Object, arguments);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to make this Object.assign.apply(null, arguments) (shouldn't matter but makes more sense when compiling to use object-assign. `_assign.apply(null, arguments)

@gaearon
Copy link
Collaborator

gaearon commented Apr 7, 2016

At least these days we know how to name things.

@@ -65,6 +81,9 @@ var React = {
DOM: ReactDOMFactories,

version: ReactVersion,

// Hook for JSX spread, don't use this for anything else.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe state in the comment that this is deprecated and evil to use?

@sebmarkbage
Copy link
Collaborator

I originally confused this with https://github.com/facebook/react-native/blob/master/Libraries/ReactNative/ReactNative.js#L96 and got overexcited to kill this too. I should've deprecated it first.

@zpao zpao force-pushed the re__spreadwarn branch 2 times, most recently from efc4042 to 7072559 Compare April 8, 2016 00:05
@zpao zpao added this to the 15.0.1 milestone Apr 8, 2016
@zpao
Copy link
Member Author

zpao commented Apr 8, 2016

Can we make it say something about TypeScript and JSTransformer specifically so people know how to upgrade?

Better? I figured we could just link to a page on the wiki that discusses typescriupt and coffeescript specifically instead of putting it in the warning.

@sebmarkbage
Copy link
Collaborator

👍

@facebook-github-bot
Copy link

@zpao updated the pull request.

@zpao zpao force-pushed the re__spreadwarn branch from 7072559 to fc1cfb6 Compare April 8, 2016 00:30
@zpao zpao merged commit 516c1d8 into facebook:master Apr 8, 2016
@zpao
Copy link
Member Author

zpao commented Apr 8, 2016

Just going to point that fb.me url here for the time being. We can change it later, but I'm optimizing for the path which gets me to a drink fastest.

@facebook-github-bot
Copy link

@zpao updated the pull request.

zpao added a commit to zpao/react that referenced this pull request Apr 8, 2016
Add back React.__spread and make it warn
(cherry picked from commit 516c1d8)
@glenjamin
Copy link
Contributor

Seeing a few people pop up in support channels with this issue - might be worth adding a note to the v15 announcement?

Medium-term: I wonder if there's a good way to automatically check compatibility with widely used downstream projects against RCs. I know really that should be downstream's responsibility - but I wonder if there's anything React's team/community can do to help.

@m4tthumphrey
Copy link

This bit me on the bum when I just upgraded to 15.0. I get that it's been deprecated/removed but even after reading this thread I'm a confused on what I need to do fix it?

Edit: I think I need to point out that I'm not using React.__spread myself; my render method looks like

return (
  <input type="text" value={value} onChange={onChange} className={classeNames} {...other} />
);

and the ...other transpiled to

React.spread(..)

Does this make a difference?

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2016

@m4tthumphrey

This is not a problem with your code. It's a problem with whatever tool you use to compile JSX.

If you are using react-tools to compile JSX, please switch to Babel. That project was unsupported for more than a year.

The fix will be out today because TypeScript still depends on it but this method is still deprecated so please try to migrate to tools that don't use it.

@m4tthumphrey
Copy link

@gaearon Thanks Dan, thats what I thought. Forgive me but how do I switch from using react-tools to babel? I already tried this morning after reading through the blog post but didn't really know what I was doing..?

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2016

How do you compile JSX?

@m4tthumphrey
Copy link

@gaearon

webpack loaders..

module.exports = {
  entry: './js/app.js',
  output: {
    filename: './bundle.js',
    path: './static/'
  },
  module: {
    loaders: [
      {
        test: /\.js$/,
        loader: 'jsx?harmony'
      },
      {
        test: /\.(png|jpg|gif)$/,
        loader: 'url?limit=8192'
      },
      {
        test: /\.css$/,
        loader: 'style!css'
      }
    ]
  }
};

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2016

You are using jsx-loader which uses deprecated react-tools. Replace it with babel-loader. Then configure Babel to use es2015 and react presets and you should be good.

@m4tthumphrey
Copy link

Ahh I see, great than you Dan.

On 8 April 2016 at 16:01, Dan Abramov notifications@github.com wrote:

You are using jsx-loader which uses deprecated react-tools. Replace it
with babel-loader. Then configure Babel to use es2015 and react presets
and you should be good.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6444 (comment)

@dsifford
Copy link

@gaearon

You are using jsx-loader which uses deprecated react-tools. Replace it with babel-loader. Then configure Babel to use es2015 and react presets and you should be good.

I'm doing these things now and still getting the depreciation warning. Is this to be expected until Typescript fixes whatever internals they need to fix? Or should I investigate further?

@gaearon
Copy link
Collaborator

gaearon commented Apr 11, 2016

I'm doing these things now and still getting the depreciation warning. Is this to be expected until Typescript fixes whatever internals they need to fix?

If you use TypeScript, yes, you will need for them to release the fix.

@dsifford
Copy link

Ok great that's what I assumed. Thanks for the clarification 👍

@DanielRosenwasser
Copy link
Contributor

As a heads up, this should now be fixed in TypeScript 1.8.10.

@zpao
Copy link
Member Author

zpao commented Apr 12, 2016

Thanks @DanielRosenwasser, I appreciate the quick turn around! Sorry for the surprise breakage.

@DanielRosenwasser
Copy link
Contributor

No worries - I've been trying to keep a better eye on the activity of the repo since this all first went down. We really appreciate how quick the React team was to get a fix out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.