-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Unfreeze the react-dom/server interface #11531
Conversation
i did see tests fail when the change was incomplete, so getting them passing again gives me as much confidence that this is completed successfully as i can get without deeper context of the project. since this change should not change actual behaviors, it didn't seem appropriate to add any additional tests for this change, but if you'd like anything additional included with the change, just let me know. |
packages/react-dom/server.browser.js
Outdated
module.exports = require('./src/server/ReactDOMServerBrowser'); | ||
var ReactDOMServer = require('./src/server/ReactDOMServerBrowser'); | ||
|
||
module.exports = ReactDOMServer.default |
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.
Can you please add a similar TODO comment as we have in other such entry points?
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.
And it's probably worth mentioning this issue too. In case somebody tries to change it back.
Have you verified this fixes your project? |
Sorry, I should have included that, yes this does fix my project without any other modifications. I'll get those comments included as soon as I'm back to a computer later today. |
comments are added. let me know if you would like any wording tweaked |
packages/react-dom/server.browser.js
Outdated
var ReactDOMServer = require('./src/server/ReactDOMServerBrowser'); | ||
|
||
// TODO: decide on the top-level export form. | ||
// This is hacky but makes it work with both Rollup and Jest and handles https://github.com/facebook/react/issues/11526 |
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.
I would split the new verbiage into a separate line.
// Note: when changing this, also consider <link>
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.
sure thing. updated
packages/react-dom/server.node.js
Outdated
|
||
// TODO: decide on the top-level export form. | ||
// This is hacky but makes it work with both Rollup and Jest | ||
// Note: when changing this, also consider https://github.com/facebook/react/issues/11526 |
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.
Actually let's move this note to export default {
(in both cases).
Because right now it would be possible to remove default
again and tests/build would still pass.
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.
sure, that makes sense. updated
this allows stubbing of the exposed named functions, as was possible before v16.1 fixes #11526
* Unfreeze the react-dom/server interface this allows stubbing of the exposed named functions, as was possible before v16.1 fixes facebook#11526 * Fix missing version export * Fix missing version export * Whitespace
this allows stubbing of the exposed named functions, as was possible before v16.1
fixes #11526