Skip to content

Build ReactDOM browser builds into react-dom/dist #4901

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
Sep 28, 2015

Conversation

zpao
Copy link
Member

@zpao zpao commented Sep 17, 2015

Fixes #4841

@zpao zpao force-pushed the npm-react-dom-dist branch from 89f721e to ff542de Compare September 18, 2015 21:02
@zpao
Copy link
Member Author

zpao commented Sep 21, 2015

I realized over the weekend that if we remove react-dom from dist/ in the react package, we're probably going to have to have 2 packages on cdnjs and jsdeliv

Thoughts on leaving react-dom in the react package for now (and putting it into the new package as well)?

@gaearon
Copy link
Collaborator

gaearon commented Sep 21, 2015

Thoughts on leaving react-dom in the react package for now (and putting it into the new package as well)?

I don't fully understand what you're suggesting but please keep in mind the library authors who provide UMD builds. Some libraries depend on just react, others also depend on react-dom. (Disregard my comment if everything is fine—I just wanted to remind this is something to keep in mind.)

@zpao
Copy link
Member Author

zpao commented Sep 21, 2015

Ultimately there should be no trace of react-dom left in the reactpackage. The PR as it stands now takes that step for the UMD build, removing dist/react-dom.js from the react npm packages and putting it into the react-dom npm package. This lines up with what we've done for the the rest of the npm packaging but would make it more difficult to use React in some cases.

Right now the CDN autoupdaters maps libraries on their end to a single package on npm (or bower is possible too I think). That means if we do remove dist/react-dom.js it won't show up on https://cdnjs.com/libraries/react/ and we'd have another library on the CDN (eg https://cdnjs.com/libraries/react-dom).

I'm not sure what you mean by keeping the library authors in mind…

@gaearon
Copy link
Collaborator

gaearon commented Sep 21, 2015

Ah, I get it now.

I'm not sure what you mean by keeping the library authors in mind…

I phrased it poorly. There needs to be a way to build UMD libs depending on React. Previously, we'd just peer-depend on react NPM package, and set react: 'React' as external in UMD build so people can use our libraries globally with precompiled React.

Now, we peer-depend on react and (in some cases) react-dom NPM package. The question is what to put in externals configuration for UMD builds. Will there be two React precompiled builds, with react.js and react-dom.js? This confuses me because react-dom NPM package is just a link now (for compat reasons) and I expect the actual move to occur with 0.15. Do libraries whose UMD builds currently depend on react: 'React' need to adjust for 0.14? Is react.js going to be “React with deprecated react-dom” inside in 0.14, and react-dom.js will just expose the “real” react-dom from it? Should library authors ask that, when you use UMD build of some component, you include react-dom.js as a global React instead of react.js? Or is react-dom.js going to expose ReactDOM as global or something of the sorts?

Sorry for the braindump, feel free to ignore me. :-) I'll happily wait for 0.14 to actually figure this out.

@zpao
Copy link
Member Author

zpao commented Sep 21, 2015

@gaearon See #4814 for the addition of dist/react-dom.js. We're sidetracked from the point of this PR but I'll explain it since we're all here anyway.

dist/react.js - The same as it was before. But as with require('react') in node, this will warn when using deprecated APIs, eg React.render.
dist/react-with-addons.js - Also the same
dist/react-dom.js - This exposes a new global ReactDOM with the APIs there without warnings (ReactDOM.render). With 0.14 it requires that either of the 2 above files are also already on the page (because shared state and all that fun stuff). This is the reason we added __SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED (we didn't trick you). In only has the APIs that require('react-dom') has.. This should place nicely with UMD (you can see the wrapper in the above PR). This is the file in question in this PR.

This confuses me because react-dom NPM package is just a link now (for compat reasons) and I expect the actual move to occur with 0.15.

I hope the above makes it clear that the UMD build is also just a link right now with the plan to separate better so that this isn't the case.

Do libraries whose UMD builds currently depend on react: 'React' need to adjust for 0.14?

The don't need to, but if they are using deprecated APIs they may want to. In that case react-dom: ReactDOM would need to be added to that externals map and you would tell users to have both files on their page.

Does that all make sense?

I'll happily wait for 0.14 to actually figure this out.

Noooo that's why we have RCs, to try to get this all sorted out before we screw it up 😉. If we ship and you tell us we did it wrong after the fact, then that's no good.


So the root of my question was just what to do with dist/react-dom.js - should it stay in the react npm package and duplicate it into react-dom package or if it should exclusively be the react-dom package and see what we can do about CDNs?

@sophiebits
Copy link
Collaborator

I thought cdnjs used our bower package. If it doesn't, I think it can.

@sophiebits
Copy link
Collaborator

(y)

@zpao
Copy link
Member Author

zpao commented Sep 25, 2015

cdnjs can use github or npm. Using github is exactly how bower works anyway so yes that's possible.
jsdelivr can use github or npm or bower. Also possible.

edit: it's not clear how these will picks versions… both are using npm right now and seem to just use the latest tag as the source of truth. Using our bower repo will be interesting if they don't pre-process the versions to look for "stable-ness" like bower does. (eg, would rc be the default when looking at the React page?)

Based on that response you're saying "remove it from react/dist" and get the CDNs to update differently.

Eventually we may want to stop doing that so we can version these things separately. Then we would want multiple CDN packages. But we can cross the bridge later.

@sophiebits
Copy link
Collaborator

Oh, I had sort of assumed this did remove from react/dist. I guess not…

@zpao
Copy link
Member Author

zpao commented Sep 25, 2015

It does - that's the change in grunt/tasks/npm-react.js (I've done a terrible job of communicating what's happening)

@gaearon
Copy link
Collaborator

gaearon commented Sep 26, 2015

@zpao Thanks for the explanation. Pieces came together in my head now. :-)

@zpao zpao mentioned this pull request Sep 28, 2015
17 tasks
@zpao zpao added this to the 0.14 milestone Sep 28, 2015
zpao added a commit to zpao/cdnjs that referenced this pull request Sep 28, 2015
In facebook/react#4901 we're going to be moving `react-dom.js` outside of the `react` npm package. For now we'd like to keep that file still in the same project here on the CDN (and we're still distributing it with the `react` bower package). In order to do that we need to point the autoupdater at the repo we use for bower instead of npm.

I think the only difference here is that we store RCs here. We always published those to npm as well but because we never used the `latest` tag, they never ended up here.
zpao added a commit to zpao/jsdelivr that referenced this pull request Sep 28, 2015
In facebook/react#4901 we're going to be moving `react-dom.js` outside of the `react` npm package. For now we'd like to keep that file still in the same project here on the CDN (and we're still distributing it with the `react` bower package). In order to do that we need to point the autoupdater at the repo we use for bower instead of npm.

I think the only difference here is that we store RCs here. We always published those to npm as well but because we never used the `latest` tag, they never ended up here.
@zpao
Copy link
Member Author

zpao commented Sep 28, 2015

Alright, jsdelivr already merged their change. I don't expect problems with cdnjs. So I'm going to merge now and if any issues arise, it's just code and we can change it.

zpao added a commit that referenced this pull request Sep 28, 2015
Build ReactDOM browser builds into react-dom/dist
@zpao zpao merged commit 1b5cd36 into facebook:master Sep 28, 2015
@zpao zpao deleted the npm-react-dom-dist branch October 13, 2015 16:39
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.

4 participants