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 #1357. Don't append "px" suffix to CSS string values. #5140

Merged
merged 1 commit into from Oct 14, 2015

Conversation

Projects
None yet
6 participants
@pluma
Copy link
Contributor

commented Oct 12, 2015

This implements the change discussed in #1357 by simply not appending "px" if the CSS value is already a string.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2015

👍 lgtm, @spicyj

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2015

We need to do this with a warning for a release to give people the chance to upgrade their code.

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

@spicyj Okay. What can I do? Add a warning(false, "message goes here") to it?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2015

Yeah. It's a little tricky because ideally you want to also log the containing component and you don't want to log thousands of times so some sort of deduplication is needed (probably based on the component name and CSS property).

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

CSSPropertyOperations.setValueForStyles would be an option -- it gets called in most of the right places and has access to the DOM node and the styles. Only problem would be deduplication. And I'm not sure whether it is called on the initial render (probably not?).

Another option would be ReactDOMComponent._createOpenTagMarkupAndPutListeners but it would require looping over all the property names again and checking for numeric strings (which seems redundant).

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2015

You can change the API for CSSPropertyOperations to pass in the necessary context if that's helpful.

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

@spicyj that would be helpful but how do I avoid creating a memory leak while keeping track of what components I've already warned about? I guess I could just use extendo properties on the DOM nodes but that seems awful and would deduplicate by DOM node, not by component. And I'm not sure I want to add properties to components for that either.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2015

@pluma We probably wouldn't want to expando on DOM nodes. You could use ReactInstanceMap which is currently implemented as an expando on internal components. Or it would probably be sufficient to warn once per style-key (ie. just create an object and set object[styleKey] = true if you've already warned for that style). That would mean that fixing the warning in one component could result in a similar warning starting to be emitted from another component that has the same bug, but that is probably acceptable (@spicyj to confirm).

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2015

@jimfb ReactInstanceMap seems to be just a way to get an internal instance for a component, the internal instance itself is sealed (doesn't take arbitrary properties). I don't see any way to use that to add an object for keeping track of warned properties.

Using a single object for all would be pretty straightforward, though.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2015

@pluma You're right, looks like it is just using expando; so if you wanted to use instances, you could do something in the same vein. Or just do a single object of css keys; I don't have a strong preference.
cc @spicyj incase he has a preference.

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2015

@jimfb @spicyj I've added a warning and a test for the warning. I had to add the warning list to the ReactDOMComponent itself though.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 13, 2015

@pluma updated the pull request.

* @return {?string}
*/
createMarkupForStyles: function(styles) {
createMarkupForStyles: function(styles, context) {

This comment has been minimized.

Copy link
@sophiebits

sophiebits Oct 13, 2015

Collaborator

Could you use the name "component" instead? "Context" often means something else (http://facebook.github.io/react/docs/context.html).

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 13, 2015

Instead of doing it per component instance, can you deduplicate based on component name? Most of our existing warnings do that already. Sorry that wasn't clear. In most cases you can get ._currentElement._owner.getName() which will be the component that created it. Rendering one component 20 times shouldn't make 20 warnings in the console.

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2015

@spicyj I would love to do that but in order to warn per component I need to add the _styleWarnings property to all React components (not just ReactDOMComponents). Or maybe I'm just an idiot.

Alternatively I could just do what @jimfb suggested and warn only on the first occurrence globally.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2015

@pluma Ben is suggesting that you call component._currentElement._owner.getName() to get the NAME of the current component (the name will be a string) and use the name of the component as part of your object-key for deduplication.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 13, 2015

See, for example, ReactElementValidator:

if (memoizer[addendum]) {
return null;
}

It is true that this makes a small memory leak but it shouldn't be an issue in practice because it grows with the size of your code and doesn't grow as you render more components repeatedly (and is dev mode only).

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2015

Okay, I've adjusted the test so it just keeps record locally instead of polluting the components.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 13, 2015

@pluma updated the pull request.

}
}
}
return value.trim();

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 13, 2015

Contributor

We usually warn for a release without a functionality change, and then change functionality the release after. The reason being: if we are emitting warnings, users probably won't/shouldn't be using the "feature" that we are warning for (ie. they need to avoid the feature in order to avoid the warning). For this reason, users won't benefit from the functionality change this release, so we might as well forestall the functionality change until they would actually be able to use it (so as to avoid breaking their apps when they update React versions).

This comment has been minimized.

Copy link
@pluma

pluma Oct 14, 2015

Author Contributor

Okay. Should I just revert the original change (i.e. remove the return statement and adjust the tests and docs) then?

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

Yes, removing the return statement and adjusting the docs sounds good.

* @return {string} Normalized style value with dimensions applied.
*/
function dangerousStyleValue(name, value) {
function dangerousStyleValue(name, value, component) {

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 13, 2015

Contributor

We should probably take in a component name, since some components will be stateless and won't have instances but should still have names (I think).

This comment has been minimized.

Copy link
@pluma

pluma Oct 14, 2015

Author Contributor

What would I need to change to do that? Just pass in component._currentElement._owner.getName() directly?

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

Eeeh, now I'm having mixed feelings about this one. If we're going to make a change, we might as well add the name of the current component in addition to the owner component (to provide more context). So directly passing in something like component._currentElement._owner.getName() and component.._tag. We could do that, but it also means wiring it through createMarkupForStyles and I'm not sure it's worth it at this point.

In a perfect world, we would have the full path, which would require merging #5167 first, but we need to decide on the fate of that PR before we can merge it, and it might not be worth delaying this PR even longer.

I'm tempted to say that this is fine for now, and we can make it better after #5167. Unless @spicyj wants it changed now (or unless we merge #5167 real fast), my intuition is that maybe you just ignore this comment and we deal with stateless components and/or giving the full parent path when those things actually happen.

var One = React.createClass({
render: function() {
return this.props.inline ? <span style={{fontSize: '1'}} /> : <div style={{fontSize: '1'}} />;
}

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 13, 2015

Contributor

Linter requires trailing comma. You can run the linter by executing npm run lint

This comment has been minimized.

Copy link
@pluma

pluma Oct 14, 2015

Author Contributor

Derp. Fixed.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 14, 2015

@pluma updated the pull request.

if (__DEV__) {
if (component) {
var owner = component._currentElement._owner;
var componentName = owner ? owner.getName() : component._currentElement.type;

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

Is this line correct? It is conflating the owner name with the element type, which feels wrong to me.

I think we actually do want to print both pieces of information (the owner if it exists, and the current element's tag) in the error message. Something like a %s tag (owner: %s) was passed a numeric string value for CSS property%s(value: %s) which will be treated as a unitless number in a future version of React.

This comment has been minimized.

Copy link
@pluma

pluma Oct 14, 2015

Author Contributor

Will do.

@@ -48,7 +51,27 @@ function dangerousStyleValue(name, value) {
}

if (typeof value === 'string') {

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

I don't think this check is enough; we should not fire the warning if the string already contains the 'px' at the end. Or actually, it might not always be px (could be 'em' or something). We should only print the warning if the string value can be parsed as an integer.

}
}
}
return value.trim();
}
return value + 'px';

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

We should only append 'px' if the value can be parsed as an integer. If they specify units, we should honor those units to avoid printing '5pxpx' or '5empx' which would make no sense. We can make that change in 0.15 (ie. now), since it should break any old code since presumably no one is doing that yet (because the old code would break).

This comment has been minimized.

Copy link
@pluma

pluma Oct 14, 2015

Author Contributor

That behaviour hasn't actually changed. Otherwise the tests would be falling, anyway. If you look at my first commit you'll see that the only actual change in logic is that the reassignment has been replaced with a return statement. This only affects unitless numeric string values.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

@pluma I found a couple more things. Sorry for all the churn on this diff, but it is looking good now :). I think we're close!

Also, you'll want to squash all the commits into a single commit before we merge. You can use git rebase -i HEAD^^^^^^ to assist in squashing the commits and then do a git push -f to override the branch rather than adding new commits to it.

@pluma pluma force-pushed the pluma:patch-1 branch from 999b70a to 69e677e Oct 14, 2015

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

@jimfb I've cleaned up the PR, reverted the original commit and adjusted the warning.

When this PR is merged, should I create another PR with the original change (and the warning removed) that can later be merged into 0.16?

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 14, 2015

@pluma updated the pull request.

@pluma pluma force-pushed the pluma:patch-1 branch from 69e677e to a299a36 Oct 14, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 14, 2015

@pluma updated the pull request.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

@pluma Na, you don't need to create a PR for 0.16 yet. The PR would just get old / we would forget about it / there would be merge conflicts / etc. Once we're getting close to the 0.15 release date (you'll know because we will create a 0.15 umbrella issue and start checking things off) then we can start generating PRs for 0.16.

I think the only two remaining changes for this PR (in 0.15) are:

  • We should not warn unless the string value specified is a unitless number in the form of a string. That is to say: it parses as an integer, as oppose to being something like 5px or 5em.
  • We should not append 'px' unless it is a unitless number. This is a non-breaking change (so we can add it now) because previously it would have resulted in something like 5pxpx or 5empx which would have made no sense.
@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

@jimfb both those points are already addressed by the pre-existing isNaN check in line 47 (unlike Number.isNaN, plain isNaN also returns true for values that aren't NaN but don't parse as numbers, like foo or even 4px). The two tests in CSSPropertyOperations-test.js lines 54 onward already test this behaviour.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

Ah, got it! Looks good to me.

jimfb added a commit that referenced this pull request Oct 14, 2015

Merge pull request #5140 from pluma/patch-1
Fix #1357. Don't append "px" suffix to CSS string values.

@jimfb jimfb merged commit 555fd46 into facebook:master Oct 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

@pluma Thanks again!

@pluma pluma deleted the pluma:patch-1 branch Oct 14, 2015

@pluma

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

You're welcome.

@joshdover joshdover referenced this pull request Apr 18, 2016

Closed

use react 15.0.1 #132

@matthetherington

This comment has been minimized.

Copy link

commented Apr 19, 2016

I'm getting a ton of warnings since upgrading to 15.0.1 due to this.

We have styles such as margin: '0' or top: '0' throughout our codebase - shouldn't the warning be skipped for zero values? The unit suffix is optional for zero values as per the CSS spec:

https://www.w3.org/TR/CSS2/syndata.html#length-units

I'll be happy to submit a PR if this is something wanted.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

We have styles such as margin: '0' or top: '0' throughout our codebase - shouldn't the warning be skipped for zero values?

Yes, this was merged, just isn’t out yet. See #6458.

@matthetherington

This comment has been minimized.

Copy link

commented Apr 19, 2016

Ah sweet.

@kylecesmat kylecesmat referenced this pull request Jul 1, 2016

Merged

Update React & Radium #14

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.