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

Simplify CSS shorthand property warning #14183

Merged
merged 1 commit into from
Nov 10, 2018

Conversation

sophiebits
Copy link
Collaborator

I figured out a simpler way to do #14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.

I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
@sebmarkbage
Copy link
Collaborator

Are we ok releasing the first one in a patch release?

I think we should probably add this warning behind a feature flag since it is likely to be very noisy.

@sizebot
Copy link

sizebot commented Nov 10, 2018

Details of bundled changes.

Comparing: 961eb65...c92e400

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js -0.3% -0.2% 718.22 KB 716.11 KB 166.14 KB 165.75 KB UMD_DEV
react-dom.development.js -0.3% -0.2% 713.53 KB 711.42 KB 164.77 KB 164.38 KB NODE_DEV
ReactDOM-dev.js -0.3% -0.3% 734.85 KB 732.57 KB 165.93 KB 165.51 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@sophiebits
Copy link
Collaborator Author

I don't think this will be very noisy. I think it basically always indicates a real bug.

@sebmarkbage
Copy link
Collaborator

In that case we should first turn it on internally and verify that assumption.

@sophiebits
Copy link
Collaborator Author

Happy to. If we want to cut a patch first we can disable it temporarily I guess.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

@acdlite One more for your revert list.

@sophiebits sophiebits merged commit b98adb6 into facebook:master Nov 10, 2018
@gaearon gaearon mentioned this pull request Nov 13, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
Copy link

@astr0sl0th astr0sl0th left a comment

Choose a reason for hiding this comment

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

Looks GTG

@gaearon
Copy link
Collaborator

gaearon commented Jul 14, 2020

@astr0sl0th This PR has been merged a year ago. Please don’t leave reviews on old PRs, you’re spamming everyone’s notifications.

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.

6 participants