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

Move React.js -> react.js & switch to @providesModule react #3031

Closed
wants to merge 2 commits into from

Conversation

gabelevi
Copy link
Contributor

@gabelevi gabelevi commented Feb 4, 2015

I replaced all require('React') with require('react'). I looked for occurrences of React.js in the codebase. I fixed the things that broke the jest and grunt tests. I'm not too familiar with this code base so please advise if you think there's something I missed.

Test plan:

grunt build
grunt test
grunt lint
npm test

The lint showed 15 errors. I check master and I still saw those 15 errors.

@zpao
Copy link
Member

zpao commented Feb 4, 2015

I think we should update https://github.com/facebook/react/blob/master/npm-react/react.js for consistency as well.

@gabelevi
Copy link
Contributor Author

gabelevi commented Feb 4, 2015

Good catch. Should I also update any older docs that mention capital R react? Like when grepping for "/React[^a-zA-Z]" I saw var React = require('react-tools/build/modules/React'); in this file

@sebmarkbage
Copy link
Collaborator

Not sure what this actually solves when this change is within the React package. If we switch to CommonJS, these will all become relative paths instead of providesModule, which will presumably break Flow inside React Core anyway. The name of the top level is then determined by the package name (or magic dir internally), which means that the name of the React.js source file in this package doesn't matter.

Where will the providesModule React alias go internally?

@sebmarkbage
Copy link
Collaborator

I guess this could be an incremental step, but to make this a magic dir CommonJS package, we shouldn't refer to our own package name internally. To avoid name conflicts with any internal aliases, we could renamed this providesModule to ReactClassic or something.

Then we expose this package "main" as react. Either through a magic dir or just an internal alias.

@gabelevi
Copy link
Contributor Author

gabelevi commented Feb 4, 2015

For typing React Core we probably don't want to use the library definition of React, but rather use what we can infer from React Core. Ideally we'd have some way to verify that the library definition matches what we see. But I'm sort of ok not solving that problem until a little later. You're right though, we will need a way to keep Flow from getting confused.

Where will the providesModule React alias go internally?

The providesModule React alias will have to go somewhere that it doesn't get clobbered by the deploy.

I guess this could be an incremental step, but to make this a magic dir CommonJS package, we shouldn't refer to our own package name internally. To avoid name conflicts with any internal aliases, we could renamed this providesModule to ReactClassic or something.

Then we expose this package "main" as react. Either through a magic dir or just an internal alias.

Sounds good to me

@gabelevi
Copy link
Contributor Author

I'm not 100% sure what the status is here. Are you guys ok taking this PR or is there something you need from me?

@sophiebits
Copy link
Collaborator

@zpao @sebmarkbage Did we want this in 0.13? Milestoning so we don't forget.

@sophiebits sophiebits added this to the 0.13 milestone Feb 26, 2015
@zpao zpao removed this from the 0.13 milestone Mar 9, 2015
@nmn
Copy link
Contributor

nmn commented Jul 9, 2015

Any update on this??

@sophiebits
Copy link
Collaborator

@nmn Why is this relevant to you?

@sophiebits
Copy link
Collaborator

(Didn't mean that to sound combative – just didn't think this was causing problems for anyone and would like to know what you're experiencing.)

@nmn
Copy link
Contributor

nmn commented Jul 15, 2015

@spicyj Not relevant to me really. I had a problem with flow, and I found an issue. That issue mentioned another issue, which mentioned this issue, so I asked for updates.

(My original issue in flow about to be fixed now anyway!)

@jimfb
Copy link
Contributor

jimfb commented Nov 10, 2015

@spicyj Is this still on our todo list, or is this something that we don't care about / there is no reason to fix? Can we close out the issue?

@zpao
Copy link
Member

zpao commented Nov 10, 2015

it's on the todo list

@gaearon
Copy link
Collaborator

gaearon commented Mar 26, 2016

We’re trying to close stale PRs and get better at being decisive and reviewing them quickly. I am closing because we recently created #6336 to track this, and if we decide to proceed, this PR would need significant updates anyway. Cheers!

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.

8 participants