Skip to content

Conversation

@janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jan 30, 2017

This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See #12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure processTransform throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

Test plan
Test that there are no errors in UIExplorer
Run new unit tests

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jan 30, 2017
@mkonicek
Copy link
Contributor

Do you have some example code this would break in practice?

The new validation looks reasonable, for example all of these had to be numbers in application code even before the improvement in this pull request, correct?

  • translateX
  • translateY
  • scale
  • scaleX
  • scaleY

}

function _validateTransforms(transform: Object): void {
function _validateTransforms(transform: Array<Object>): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called transforms now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prop in style is called transform so I guess this is fine, I just fixed the flow type as it was bad, flow didn't catch the error but it always was an array that is passed to this function.

* interface to native code.
*/
function processTransform(transform: Object): Object {
function processTransform(transform: Array<Object>): Array<Object> | Array<number> {
Copy link
Contributor

@mkonicek mkonicek Jan 30, 2017

Choose a reason for hiding this comment

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

Does this change the public API? Now people have to pass:

[{scale: 2}, {translateX: 10}]

Is it the same API as before? I'm confused because the Flow type changed from Object to Array<Object> but the PR description says this is only a breaking change because of stricter validation, not an API change.

And why not pass:

{scale: 2, translateX: 10}

I guess it's because the array defines the order of transforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per my previous reply, it doesn't change the API, just fix the flow type that was wrong. The array is effectively for the order of the transforms.

@janicduplessis
Copy link
Contributor Author

#12110 is a real example of an error that now gets caught by these validations (only one prop per transform object). In this case it got caught by a new validation we added in the iOS implementation, so I want to make sure we validate everything in JS to avoid different errors on different platforms.

@janicduplessis
Copy link
Contributor Author

The only new validation added is checks for the array size for transform and 1 prop per transform object (native ios already validated it but not android). The other change is just to also validate invalid transform props also in the validate function instead of relying on native to do it.

@mkonicek
Copy link
Contributor

Thanks for the explanation! Shipping.

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jan 31, 2017
@facebook-github-bot
Copy link
Contributor

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 1, 2017
@mkonicek
Copy link
Contributor

mkonicek commented Feb 2, 2017

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 2, 2017
@facebook-github-bot
Copy link
Contributor

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

edmofro pushed a commit to edmofro/react-native that referenced this pull request Feb 6, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 7, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
normanjoyner pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
nicktate pushed a commit to nicktate/react-native that referenced this pull request Feb 9, 2017
Summary:
This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms.

See facebook#12110 for an example of an error that now gets caught by JS validations.

Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android.

**Test plan**
Test that there are no errors in UIExplorer
Run new unit tests
Closes facebook#12115

Differential Revision: D4488933

Pulled By: mkonicek

fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants