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

Unfreeze the react-dom/server interface #11531

Merged
merged 4 commits into from Nov 13, 2017
Merged

Unfreeze the react-dom/server interface #11531

merged 4 commits into from Nov 13, 2017

Conversation

travi
Copy link
Contributor

@travi travi commented Nov 11, 2017

this allows stubbing of the exposed named functions, as was possible before v16.1

fixes #11526

@travi
Copy link
Contributor Author

travi commented Nov 11, 2017

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.

module.exports = require('./src/server/ReactDOMServerBrowser');
var ReactDOMServer = require('./src/server/ReactDOMServerBrowser');

module.exports = ReactDOMServer.default
Copy link
Collaborator

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?

Copy link
Collaborator

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.

@gaearon
Copy link
Collaborator

gaearon commented Nov 12, 2017

Have you verified this fixes your project?

@travi
Copy link
Contributor Author

travi commented Nov 12, 2017

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.

@travi
Copy link
Contributor Author

travi commented Nov 12, 2017

comments are added. let me know if you would like any wording tweaked

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
Copy link
Collaborator

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing. updated


// 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
Copy link
Collaborator

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.

Copy link
Contributor Author

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
@facebook facebook deleted a comment from baby2girl Nov 13, 2017
@gaearon gaearon merged commit 901a091 into facebook:master Nov 13, 2017
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
* 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
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.

Transpilation in v16.1.0 freezes the react-dom/server interface
3 participants