-
Notifications
You must be signed in to change notification settings - Fork 49.3k
Add additional secret property to build artifact for react-dom-server #5381
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
Conversation
Update eslintrc, travis build and add task to Gruntfile
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
I'm happy to help address this in a different way that makes more sense longer term, please comment or let me know how folks think this should be addressed. For now living with warnings is fine too. :) |
Alright, we're going to do this - thanks for making it easy for us :). I'll give it a closer look shortly but I wanted to CC some interested parties so they know. @Download @aantono @JaRail @Daniel15 @aantono. We won't be building a combined bundle so you'll need to load 3 files into your environment (eg Nashorn) or build your own bundle that exposes globals. That's an exercise we'll leave to the reader (I did it too) |
Cool 👍 @zpao, thanks |
@@ -65,5 +65,6 @@ assign(React, { | |||
}); | |||
|
|||
React.__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = ReactDOM; | |||
React.__SECRET_DOM_SERVER_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = ReactDOMServer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SECRET_DO_NOT_USE_OR_YOU_WILL_BE_FIRED
made sense when we were migrating and you really would break stuff. But if we're adding this as an explicit hook to allow various environments, it shouldn't be a severe variable name (IMHO) - Or if this is just for the 0.14.3 update, perhaps it makes sense. cc @zpao @spicyj for thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is that the react-dom-server.js
package we provide uses this secret key, but you're meant to never type it in your own code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this is fine as is. We are not planning on making these properties condoned in any public use. They are subject to breaking (and I might just change them in the next release to ensure this does break)
This looks good, let's do it. Thanks again! |
Add additional secret property to build artifact for react-dom-server
I think line 26 of react-dom-server.js should be g.ReactDOMServer = f(g.React); |
@aantono Yes you are correct. Following up |
- Export to the right variable - Simplify lintignore - Fix cURL command for TravisCI
Thanks, this looks good to me! I created reactjs/React.NET#197 to use this in ReactJS.NET. |
Great stuff this!! Thank you all! |
…package Add additional secret property to build artifact for react-dom-server (cherry picked from commit c07b304)
Followup to facebook#5381 (cherry picked from commit 60cba8f)
…package Add additional secret property to build artifact for react-dom-server (cherry picked from commit c07b304)
Followup to facebook#5381 (cherry picked from commit 60cba8f)
Related #5134 and #5311.
In upgrading to React 0.14.2 today, I found that methods for rendering static markup aren't available as UMD artifacts anymore. We've been using this for HTML rendering in the browser in a handful of places where we're doing interop with other libraries and need plain HTML as part of an existing API. With React's awesome release process, this is only a warning for now.
I read around a bit but couldn't quite tell what the direction folks wanted to go with the build setup here. In my poking around I just made this PR as a workaround, adding another secret property to expose
ReactDOMServer
, and a build step to produce this a UMD artifact in the same way thatreact-dom
is built in #4814. This PR likely won't be useful, and closing it right away makes sense to me, but if anyone else needs a one-off build for this I figured it's worth posting.Longer term, I'd second the idea that this rendering code has uses beyond just server rendering, and that it may be worth including in
ReactDOM
as @jimfb suggested in #5311 (comment).