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

Fix missing else in ReactDOMServerFormatConfig.js #22431

Closed
wants to merge 1 commit into from

Conversation

Hyperion101010
Copy link

Removed else statement from ReactDOMServerFormatConfig.js
Else statement was redundant since value is set anyways.
Solves bug 22309.

Summary

This PR removes redundant code from the ReactDOMServerFormatConfig.js file.
As discussed here 22309

How did you test this change?

Tested it using
yarn prettier
yarn linc
yarn build and test

@sizebot
Copy link

sizebot commented Sep 26, 2021

Comparing: d9fb383...3c7d297

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 130.22 kB 130.22 kB = 41.45 kB 41.45 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 133.04 kB 133.04 kB = 42.42 kB 42.42 kB
facebook-www/ReactDOM-prod.classic.js = 412.58 kB 412.58 kB = 76.37 kB 76.37 kB
facebook-www/ReactDOM-prod.modern.js = 401.21 kB 401.21 kB = 74.67 kB 74.67 kB
facebook-www/ReactDOMForked-prod.classic.js = 412.58 kB 412.58 kB = 76.37 kB 76.37 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 3c7d297

Copy link
Contributor

@akgupta0777 akgupta0777 left a comment

Choose a reason for hiding this comment

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

The comment should be
//children is always a string

@Hyperion101010
Copy link
Author

Hyperion101010 commented Sep 27, 2021

@akgupta0777 fixed the comment and solved conflict.

Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

I suggested a few changes. Also, I'd suggest rebasing and squashing this into a single commit, because merge commits are discouraged in PRs. There are many ways to do this. Here's one way:

git checkout fix-22309
# rewind your branch to before your first commit
# If you've made more commits, replace "4" with the number of commits in your PR
git reset HEAD~4
# stash all changes in those commits
git stash --include-untracked
# rebase this branch with the latest changes from upstream
git remote add upstream https://github.com/facebook/react.git
git pull upstream main
# stage your changes to the one file you changed
git checkout stash@{0} -- packages/react-dom/src/server/ReactDOMServerFormatConfig.js
# undo the checkout, leaving the changed file in your working directory
git reset
#
# NOW, MAKE YOUR ADDITIONAL EDITS FROM CODE REVIEW AND SAVE THEM
#
# commit changes you made
git add packages/react-dom/src/server/ReactDOMServerFormatConfig.js
git commit -m "remove unnecessary code"
# finally, force-push to your PR branch which will clean up history
git push -f

@Hyperion101010
Copy link
Author

@justingrant all fixes are done, please review once.

Copy link
Contributor

@justingrant justingrant left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@Hyperion101010
Copy link
Author

What is the procedure to merge this PR? @justingrant

@justingrant
Copy link
Contributor

One of the maintainers will have to review it. There's 200+ open PRs so it might be a while. But this is a narrowly-scoped PR so it's unlikely to get stale soon. Someone should review & merge it sooner or later.

@Hyperion101010
Copy link
Author

@crypt096 are you helping in maintaining reactjs?

@akgupta0777
Copy link
Contributor

@bvaughn please review this PR.

@Hyperion101010
Copy link
Author

@bvaughn any updates on this?

Copy link

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Copy link

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@github-actions github-actions bot closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants