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

rename react-addons-dom-factories to react-dom-factories #9780

Merged
merged 2 commits into from
May 25, 2017

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented May 25, 2017

what is the change?:
grep -r "react-addons-dom-factories" ./src and the same in ./packages
then %s /react-addons-dom-factories/react-dom-factories/gc

why make this change?:
React dom factories were never part of the 'addons' and we want to minimize the
whole 'addons' thing, since we are deprecating the React.addons.* API.

test plan:
yarn test
and manually look at the README that was updated.

issue:
#9398

**what is the change?:**
`grep -r "react-addons-dom-factories" ./src` and the same in `./packages`
then `%s /react-addons-dom-factories/react-dom-factories/gc`

**why make this change?:**
React dom factories were never part of the 'addons' and we want to minimize the
whole 'addons' thing, since we are deprecating the `React.addons.*` API.

**test plan:**
`yarn test`
and manually look at the README that was updated.

**issue:**
facebook#9398
@flarnie flarnie added this to the 15.6 milestone May 25, 2017
@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

Note to self: cherry-pick onto master

@flarnie flarnie changed the title react-addons-dom-factories rename react-addons-dom-factories to react-dom-factories May 25, 2017
@flarnie flarnie mentioned this pull request May 25, 2017
49 tasks
Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

The build scripts are already configured correctly?

@gaearon
Copy link
Collaborator

gaearon commented May 25, 2017

Note to self: cherry-pick onto master

Do we need to? I thought this package won't receive more updates.


## Example

```javascript
import ReactDOM from 'react-dom';
import DOM from 'react-addons-dom-factories'; // ES6
import DOM from 'react-dom-factories'; // ES6
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can either remove // ES6 here or provide an ES5 example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops - just noticed this. will fix.

@flarnie
Copy link
Contributor Author

flarnie commented May 25, 2017

@spicyj

The build scripts are already configured correctly?

I don't know. Let's figure it out. :)

@gaearon

Do we need to? I thought this package won't receive more updates.

You are correct.
I was thinking that updating the package README would be good, in case it confuses people to see the wrong name there. https://github.com/facebook/react/tree/master/packages/react-dom-factories
Hopefully we'll have dropped support and won't be warning for this at all in 16.0, but just on the off-change that we delay actually removing some of these APIs I'd like the warnings to be the same as in 15.6.

flarnie added a commit that referenced this pull request May 25, 2017
Remove a comment saying `es6`, since this is potentially unclear. We aren't
contrasting to an es5 example here.

Thanks to @gaearon for catching this in review of
#9780
@gaearon
Copy link
Collaborator

gaearon commented May 25, 2017

Ah okay. I thought we can just delete that folder for 16.0.

@flarnie flarnie merged commit 6086a22 into facebook:15.6-dev May 25, 2017
@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented May 27, 2017

I notice that both react-addons-dom-factories and react-dom-factories are not published in npm,but the React 16 warning it now,so I want to know is there any suggestion to solve this?Thanks.

@gaearon
Copy link
Collaborator

gaearon commented May 27, 2017

Not sure what you mean. Afaik this warning hasn't made it into any release.

@flarnie
Copy link
Contributor Author

flarnie commented May 27, 2017

@NE-SmallTown might have seen that the warning was merged into master as well as 15.6, in #8356 so they are right that it's present in the latest unreleased 16.0-alpha.

afaik we have not done an alpha-release of the 16 branch yet that includes that warning. So @gaearon is also correct.

Instead of cherry-picking this onto master, we should do a commit that removes it and the warning both on the 16.0 branch. I'll open an issue to track that. (later this weekend probably)

Also I expect to publish the react-dom-factories package soon, hopefully that mitigates the concern.

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented May 27, 2017

Yea,I am using react 16 now.But you say 16 not includes this warning,I think this is not true because I get this warning.

And I find the src code of this in the React.js file

@flarnie
Copy link
Contributor Author

flarnie commented May 27, 2017

I believe you @NE-SmallTown - now I'm curious how it's happening.

Which of the 16 alpha releases are you using? Or are you using a build directly from the react master branch? If you have pulled the recent React code directly from master any time after Apr. 24th then you would indeed get the inactionable warning. #8356

I think the latest alpha-release of 16.0 is https://github.com/facebook/react/releases/tag/v16.0.0-alpha.3 and when I check the source code I don't see it there either.

@gaearon
Copy link
Collaborator

gaearon commented May 27, 2017

Alpha 3 is definitely not the latest, we released at least alpha 8 (and maybe more). I don't think we documented them anywhere but you can check what react@next resolves to.

Generally alphas may contain unactionable warnings etc. We don't recommend anyone to use React alphas outside of React Native.

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented May 28, 2017

@flarnie

Which of the 16 alpha releases are you using?

I am using alpha-12.0.I don't specify concrete version,I just use npm install react@next,and then the version is alpha-12.0(IIRC one or two weeks ago it is 11.0).

Or are you using a build directly from the react master branch?

No,only when I learn fiber in the fiber-debugger directory I will do that.

@gaearon

Generally alphas may contain unactionable warnings etc. We don't recommend anyone to use React alphas outside of React Native.

Yea,don't worry,please trust that I am not blaming or complaining anyone or anything.I just want to experience fiber and some new features in advance.

flarnie added a commit to flarnie/react that referenced this pull request Jun 7, 2017
Remove a comment saying `es6`, since this is potentially unclear. We aren't
contrasting to an es5 example here.

Thanks to @gaearon for catching this in review of
facebook#9780
flarnie added a commit to flarnie/react that referenced this pull request Jun 7, 2017
**what is the change?:**
`grep -r "react-addons-dom-factories" ./src` and the same in `./packages`
then `%s /react-addons-dom-factories/react-dom-factories/gc`

**why make this change?:**
React dom factories were never part of the 'addons' and we want to minimize the
whole 'addons' thing, since we are deprecating the `React.addons.*` API.

**test plan:**
`yarn test`
and manually look at the README that was updated.

**issue:**
facebook#9398
@flarnie flarnie deleted the fixNameOfReactDomFactories branch May 25, 2018 17:51
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.

5 participants