Skip to content

Adding early support for React v0.13.0 #3542

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

Merged
merged 1 commit into from
Jan 28, 2015

Conversation

jbrantly
Copy link
Contributor

React v0.13 is set to come out very soon. This adds early support for React v0.13 by versioning the new files (this should allow installation through TSD without messing up existing functionality). This will allow us to get a headstart on modeling v0.13. This PR is not meant to be a complete modeling but rather to provide a base on which to collaborate and tweak with further PRs.

In particular this PR adds the most significant change which is supporting ES6 classes. ES6 classes (aka "modern") have a slightly different API than classes created through React.createClass (aka "classic"). Also many APIs are deprecated in modern such as replaceState, getDOMNode and various prop methods.

This also includes slightly modified tests. Most existing functionality seems to work without issue (in other words I don't think the upgrade path is particularly difficult if it's even necessary at all).

Misc notes

The existing Component interface needed to be changed in order to support extending from the React-provided base class which is called React.Component. Component now provides the base class implementation. see https://github.com/facebook/react/blob/master/src/modern/class/ReactComponentBase.js and https://github.com/facebook/react/blob/master/src/modern/class/__tests__/ReactES6Class-test.js

The classical implementation is now called ClassicComponent and extends Component while adding additional API methods. This reflects what happens in reality, see https://github.com/facebook/react/blob/9abd1133c94288c09512f7cf0b8a0b8136df6da7/src/classic/class/ReactClass.js#L787-L791

CompositeComponent has been removed since the concept of state is present from the very beginning (ex React.Component.setState)

Additional information regarding propTypes and default props: https://github.com/facebook/react/blob/master/src/modern/element/__tests__/ReactJSXElementValidator-test.js

Paging @pspeter3 and @vsiao

@jbrantly
Copy link
Contributor Author

I've fixed the --noImplicitAny issue with the tests. Didn't realize it was run with that option.

Also, in the interest of full transparency, I'd like to point out that I added my company (AssureSign) to the header since I've spent a not-insignificant amount of company resources putting this together.

@pspeter3
Copy link
Contributor

I'm really excited about this! Do you know when React 0.13 is supposed to land? Beyond that, I'd really like to work together to make context type safe. I'm not sure if it is feasible to add support for child contexts easily but it seems like a valuable use case.

@jbrantly
Copy link
Contributor Author

Its been mentioned publically "hopefully around the conf" which starts Wed. I would be surprised if it wasnt launched this week.

Regarding contexts, I agree it would be ideal to support those. One problem with contexts is that they change from instance to instance (so specifying on the class might not make sense, but then again it may work for most cases). The other problem is that they are a merge of all contexts above them from my understanding and so modeling in typescript will not be ideal until intersection types are added (although, again, in the short term modeling simple cases might be good enough). This PR doesn't address them. I would suggest we merge this roughly as-is (assuming no bugs or design flaws) and then add context support in a later PR.

@jbrantly
Copy link
Contributor Author

I added context to Component. That way at least you can access this.context even if it's not type-checked. That was an oversight on my part, thanks!

To whoever does merges: I can squash if you'd like.

@jbrantly
Copy link
Contributor Author

I was also just informed of this commit which adds TypeScript class tests to React core: facebook/react@6c145c3

@pspeter3
Copy link
Contributor

That commit is awesome! I'm excited.

@pspeter3
Copy link
Contributor

Thanks for updating the context!

@jbrantly
Copy link
Contributor Author

One problem with contexts is that they change from instance to instance (so specifying on the class might not make sense, but then again it may work for most cases).

The more I think about this the more it's a non-issue. There really is no other way to model it. Additionally React's contextTypes functionality seems to scope what is available in the context to the class anyways.

Lastly, looking at https://github.com/facebook/react/blob/54c82da15f6b4717425edbf68e23ae82583a50af/src/classic/__tests__/ReactContextValidator-test.js you can see that context is actually used in lifecycle methods. All of these things together has convinced me that you're right, context should be added as a generic parameter to the class. So I've added that in the latest commit.

There are a couple of things to point out:

Child context is done through implementing React.ChildContextProvider<C> since child context is different from your context and therefore needs a new generic. However, since for ES6 classes the childContextTypes property needs to be static and you can't add a static property through an interface I've simply added that to the base class.

Secondly, I think there is a bug in TS where using static propTypes = {} in ES6 classes doesn't work correctly due "missing indexer". I can't figure out a way around the issue. If someone else can, great! There is commented-out code in the tests which shows the issue. I've opted to keep it as-is for now which means adding propTypes, contextTypes, and childContextTypes must be done after-the-fact by assigning directly to the class. The alternative would be to make those properties any.

type ReactType = ComponentClass<any, any, any> | string;

interface ReactElement<P> {
type: ReactType;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably use ComponentClass<P, any, any> | string instead of the type alias since type aliases don't support generics

@vsiao
Copy link
Contributor

vsiao commented Jan 27, 2015

Looking good! Thanks for taking the time to do this. 👍 after addressing inline comments

@pspeter3
Copy link
Contributor

Does making the ChildContextProvider a class cause run time issues since there is not an actual class in the React library? Also I believe the indexer issue is that you want

static propTypes = <ValidationMap<any>>{}

or

static propTypes = <{[key: string]: any}>{}

@jbrantly
Copy link
Contributor Author

Thanks for the in-depth reviews. I've fixed the items mentioned in the inline comments.

Regarding the propTypes thing, I feel like you shouldn't be required to cast that. For instance, this compiles fine:

var b: React.ValidationMap<any>;
b = {
    someValue: React.PropTypes.string
};

It's only when it's a property or static property of a class that it doesn't convert. I want to file an issue with TypeScript regarding it to see what they say but haven't had the time yet.

@jbrantly
Copy link
Contributor Author

@pspeter3 sorry just realized you asked about ChildContextProvider. It's not currently a class, it's an interface.

@jbrantly
Copy link
Contributor Author

@johnnyreilly @vvakame I believe this has all of the necessary signoff for merging. Would you prefer me to squash these commits?

@johnnyreilly
Copy link
Member

That would be good @jbrantly. @vvakame are you happy for this to go in?

@jbrantly
Copy link
Contributor Author

Squashed.

@pspeter3
Copy link
Contributor

👍

@pspeter3
Copy link
Contributor

It would be nice to make sure that this is correct with http://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html but I think it is and I'm excited to use it.

vvakame added a commit that referenced this pull request Jan 28, 2015
Adding early support for React v0.13.0
@vvakame vvakame merged commit 43e1aba into DefinitelyTyped:master Jan 28, 2015
@vvakame
Copy link
Member

vvakame commented Jan 28, 2015

👍
@jbrantly thanks mate!
@pspeter3 thanks a lot!
@johnnyreilly 👍

@jbrantly
Copy link
Contributor Author

jbrantly commented Feb 3, 2015

For anyone who cares....

Secondly, I think there is a bug in TS where using static propTypes = {} in ES6 classes doesn't work correctly due "missing indexer". I can't figure out a way around the issue.

See microsoft/TypeScript#1373

@pspeter3
Copy link
Contributor

pspeter3 commented Feb 3, 2015

@jbrantly I think there is a problem with how the React.Component is exported in the new model which prevents it from compiling. Specifically that you can't extend the ComponentClass interface.

@jbrantly
Copy link
Contributor Author

jbrantly commented Feb 3, 2015

@pspeter3 I don't think you should be extending ComponentClass. You can extend Component (or ClassicComponent).

@pspeter3
Copy link
Contributor

pspeter3 commented Feb 3, 2015

I'm not purposely extending ComponentClass. The Exports interface has Component: ComponentClass<any, any, any> which causes it not to work

@jbrantly
Copy link
Contributor Author

jbrantly commented Feb 3, 2015

Are you using import React = require('react')? I think the casing matters. I found that TypeScript wasn't too happy if React.Component (the type) and React.Component (the value) didn't match up, but I didn't play with it too much.

@pspeter3
Copy link
Contributor

pspeter3 commented Feb 3, 2015

Both cases actually don't work. I've been thinking about potentially renaming the react module for that reason.

@jbrantly
Copy link
Contributor Author

jbrantly commented Feb 3, 2015

Odd. I haven't had any problems with it. Take a look at https://github.com/jbrantly/reactconf/blob/master/src_ts/app.ts (worked for the conference)

@jbrantly
Copy link
Contributor Author

jbrantly commented Feb 5, 2015

@pspeter3 did you ever get this working or is there an issue?

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

Successfully merging this pull request may close these issues.

5 participants