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 module react-dom #5828

Merged
merged 5 commits into from
Oct 7, 2015
Merged

Add module react-dom #5828

merged 5 commits into from
Oct 7, 2015

Conversation

B-Stefan
Copy link
Contributor

Add the react-dom definitions for [email protected],
Definitions copied from react.d.ts

@jbrantly
Copy link
Contributor

Obviously this needs to get added one way or another for React v0.14. I had originally thought we'd include it in the normal react tree instead of making a new one here (react-dom). I can certainly see an argument for making a new one. However, there are going to be a crapton of other minor packages needed for v0.14 as well: react-addons-clone-with-props, react-addons-create-fragment, react-addons-css-transition-group, react-addons-linked-state-mixin, react-addons-pure-render-mixin, react-addons-shallow-compare, react-addons-transition-group. Do we want to make new folders for all of these as well?

I think I'm still leaning towards putting everything in a single definition file. I think it would make it much easier for developers to just get up and running, but shrug. What do others think?

cc @vsiao @pspeter3 @ngbrown

@B-Stefan
Copy link
Contributor Author

Basicly you totally right that's a mass of definitions for only little packages, I didn't thought about this.

I see only one reason for creating a folder for each package: The would be correspond the current structure of the repository: One folder for one npm module. So maybe some developer search for a react-dom folder don't find one and think that there is no definition for react-dom. Maybe they don't think that the definition is already included in the react.d.ts file.

@pspeter3
Copy link
Contributor

I'm a fan of one react folder where each of these files exist. Using the principle that of avoiding run time errors, it seems like you should only have access to a module if you explicitly include it. Ex, it would be frustrating for a developer to require react-dom because it is in the typings but then crash because it is not in your node_modules

@jbrantly
Copy link
Contributor

also pinging @johnnyreilly since he's a maintainer and has some React experience.

@johnnyreilly
Copy link
Member

I like the way in angular 1.x we've nicely separated out each of the definition files so the user can include what they need as they use it. I'd quite like a similar appraoch to be used for React unless there's a compelling reason why not.

All that said, I'm still a little on the fence...

@pspeter3
Copy link
Contributor

Given there is a precedent, it seems best follow it to maintain consistency

On Wed, Sep 16, 2015, 3:31 AM John Reilly [email protected] wrote:

I like the way in angular 1.x
https://github.com/borisyankov/DefinitelyTyped/tree/master/angularjs
we've nicely separated out each of the definition files so the user can
include what they need as they use it. I'd quite like a similar appraoch to
be used for React unless there's a compelling reason why not.

All that said, I'm still a little on the fence...


Reply to this email directly or view it on GitHub
#5828 (comment)
.

@jbrantly
Copy link
Contributor

So if I understand everyone correctly, @pspeter3 suggested the following approach:

DefinitelyTyped/react/react-dom.d.ts (note: not DefinitelyTyped/react-dom/react-dom.d.ts)
DefinitelyTyped/react/react-addons-clone-with-props.d.ts
etc

Angular 1.x also uses the same approach.

@johnnyreilly is on the fence but leaning toward the same approach
I'm on the fence but I think it's a perfectly reasonable approach

Assuming this is the case, @B-Stefan could you update the PR to put react-dom.d.ts into the react folder instead?

@jbrantly
Copy link
Contributor

As a separate issue, this doesn't address a global version. I think we would also need a react-dom-global.d.ts which exposes whatever the global API is (I don't actually know how they're handling this, I haven't looked. If they're just bundling it with the global React then maybe the existing definitions already work).

@pspeter3
Copy link
Contributor

Is there a consistent global version after these changes? I would assume no since you would have to access them through npm anyway. I would prefer not having a global version and ideally but that may not be tenable.

@rcchen
Copy link
Contributor

rcchen commented Oct 7, 2015

Bumping since React 0.14.0 was released today, can we get an update on this one?

@jbrantly
Copy link
Contributor

jbrantly commented Oct 7, 2015

I think this one could be merged if the files are moved.

I'm working on a more comprehensive v14 typings update.

johnnyreilly added a commit that referenced this pull request Oct 7, 2015
@johnnyreilly johnnyreilly merged commit 66f88ec into DefinitelyTyped:master Oct 7, 2015
@johnnyreilly
Copy link
Member

Done

@rcchen
Copy link
Contributor

rcchen commented Oct 7, 2015

Awesome, thanks @johnnyreilly! 🎈

@jbrantly
Copy link
Contributor

jbrantly commented Oct 7, 2015

Doh.... but this wasn't ready to be merged :( the files weren't moved yet.

@pspeter3
Copy link
Contributor

pspeter3 commented Oct 7, 2015

We also need to add the other modules too. I think React 0.14 type definitions will need more coordination than this.

@johnnyreilly
Copy link
Member

OK - feel free to submit another PR and ping me

@jbrantly jbrantly mentioned this pull request Oct 8, 2015
24 tasks
@alexgorbatchev
Copy link
Contributor

I think the boundaries should be at modules, if I have to npm install react-dom i expect to have to tsd install react-dom as well. Please don't mush everything together.

@alitaheri
Copy link
Contributor

I agree with @alexgorbatchev not everyone need the react-dom module, and if this has to be merged so will have many others. I think many conveniently separated modules serve better maintainability and readability than having one GOD module that has everything.

@jbrantly
Copy link
Contributor

i expect to have to tsd install react-dom as well (emphasis mine)
not everyone need the react-dom module
one GOD module that has everything

I'm wondering if there is some confusion going on here though. Just because react-dom and react-addons-* were being added to the react folder doesn't mean they suddenly pollute your app or something. There is no "god" module. You would still need to individually reference each package's .d.ts file in order to use it. It also doesn't pollute the already overpopulated DefinitelyTyped repo and there is precedent with the Angular and node typings.

However, I just did some testing with tsd (I don't really use tsd regularly) and I was under the impression that tsd install react would download all of the definition files under the react folder, but that doesn't seem to be the case. A user would have to type tsd install react/* to grab everything. I can certainly see a user thinking of tsd install react-dom before thinking tsd install react/*. There's also the argument that people are already using tsd install react-dom so taking it away could be jarring.

My (personal) goal is ease-of-use. I was under the impression that putting everything under the react folder would make things easy since someone could just use tsd install react and have access to everything they needed, but I'm starting to think that was misguided.

@alexgorbatchev
Copy link
Contributor

I didn't know about tsd install react/*. It feels counter intuitive that tsd install react installs react/react.d.ts and tsd install react/* installs everything. However, if that's the way it is already, then keeping everything under react/ folder makes sense.

Having said that, a decision needs to be made whether or not to restrict react folder to Facebook official modules only because there's a large number of other react modules already in the repo:

$ tsd query react*

 - react-bootstrap        / react-bootstrap
 - react-dnd              / react-dnd
 - react-dom              / react-dom
 - react-input-calendar   / react-input-calendar
 - react-intl             / react-intl
 - react-mixin            / react-mixin
 - react-props-decorators / react-props-decorators
 - react-redux            / react-redux
 - react-router           / react-router
 - react-spinkit          / react-spinkit
 - react-swf              / react-swf
 - react                  / react-global
 - react                  / react

@alitaheri
Copy link
Contributor

@jbrantly I did kinda confuse moving react-dom to reach folder with adding react-dom definitions to the react.d.ts file, my bad 😁

@johnnyreilly
Copy link
Member

I'm a little torn. On the one hand I like the idea that you have an equivalent tsd install react-thingy for each npm install react-thingy. But on the other hand I like the idea of all the official typings living in the same folder and being separated into different files. Users can control what they use thanks to tsconfig.json and this approach has worked pretty well for the angular 1.x typings.

I dunno. DefinitelyTyped is now so full it's becoming hard to navigate. That also tempts me in the single official folder direction...

@jbrantly
Copy link
Contributor

Having said that, a decision needs to be made whether or not to restrict react folder to Facebook official modules only

Yea, if we did keep everything under the react folder it would be "official" packages only.

@basarat
Copy link
Member

basarat commented Oct 28, 2015

I strongly want a single folder as well. Finding which definitions exist and which do not would be a pain otherwise.

@basarat
Copy link
Member

basarat commented Oct 28, 2015

Code mirror is all in a single folder and I love it https://github.com/borisyankov/DefinitelyTyped/tree/master/codemirror

@alexgorbatchev
Copy link
Contributor

👍 for single folder for official Facebook react modules

@rcchen
Copy link
Contributor

rcchen commented Oct 28, 2015

Question for proponents of the single folder approach: would you expect react-native.d.ts to be found in the react/ folder as well? Seems like people will pull down a bunch of irrelevant typings if they're only targeting web DOM or iOS/Android/etc.

@alexgorbatchev
Copy link
Contributor

With the current setup I would expect all typings for official react modules to be in react/.

@jbrantly
Copy link
Contributor

Seems like people will pull down a bunch of irrelevant typings

Yes, but that doesn't mean all of the irrelevant typings would be used. You would still need to add to tsconfig.json or a tripleslash reference.

@basarat
Copy link
Member

basarat commented Oct 28, 2015

Question for proponents of the single folder approach: would you expect react-native.d.ts to be found in the react/ folder as well? Seems like people will pull down a bunch of irrelevant typings if they're only targeting web DOM or iOS/Android/etc.

only if they manually download them all. If they tsd install react react-dom they would only get two files. The folder location does not impact how tsd works. Its just for convenience.

@jbrantly
Copy link
Contributor

The folder location does not impact how tsd works. Its just for convenience.

Holy crap you're right. That's awesome. We can get the best of both worlds.

@rcchen
Copy link
Contributor

rcchen commented Oct 29, 2015

Woah....did not know that. Full steam ahead with single folder then!

@joewood
Copy link

joewood commented Oct 29, 2015

Wouldn't it be great if we could convince our Facebook friends to take these files into their repo. Then we wouldn't need tsd at all for React.

@pspeter3
Copy link
Contributor

Potentially but it would also make it harder for us to update them.

@johnnyreilly
Copy link
Member

@joewood this isn't as far fetched as you might imagine. Facebook are more focused on using Flow (similar to TypeScript but not the same). However, one of the goals of Flow is to have support for .d.ts files.

Once that happens, then maybe it might become a possibility for FB to ship definition files with React itself. This is not something that is likely to happen anytime soon but you could try asking @spicyj / @zpao if they've any idea whether React is ever likely to ship with it's own definition files? It'd be nice!

@basarat
Copy link
Member

basarat commented Oct 29, 2015

fwiw they already ship definition files for immutablejs https://github.com/facebook/immutable-js/tree/master/type-definitions 🌹

@johnnyreilly
Copy link
Member

Good point @basarat! Ironically I've used immutable js but never actually with TypeScript.

😄

@thasner
Copy link
Contributor

thasner commented Nov 3, 2015

👍 for single folder for official Facebook react modules

jbrantly added a commit to jbrantly/DefinitelyTyped that referenced this pull request Nov 5, 2015
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.

10 participants