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

Replace 2 occurrences of string in type ReactTestRendererJSON #8816

Merged
merged 2 commits into from Jan 23, 2017

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Jan 17, 2017

While studying implementation details of react-test-renderer, type ReactTestRendererJSON contradicted instead of confirmed what I had observed in Jest.

To verify misleading Flow errors, I temporarily pasted an object as if from renderer.create(<th colSpan={2}>{13}</th>).toJSON() into ReactTestRendererFiber.js and ReactTestRendererStack.js:

const th: ReactTestRendererJSON = {
  type: 'th',
  props: { colSpan: 2 },
  children: [13],
};

Currently the type is neither exported nor has an opportunity to report incorrect errors, but it could confuse people who read it as the specification of the object format from the test renderer.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2017

@iamdustan Can you please review this?

Copy link
Contributor

@iamdustan iamdustan left a comment

Choose a reason for hiding this comment

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

I’m 90% certain this is okay, but want to confirm the possibly subtle differences between fiber and stack and the handling of numbers.

children : null | Array<ReactTestRendererNode>,
$$typeof ?: Symbol, // Optional because we add it with defineProperty().
|};
type ReactTestRendererNode = ReactTestRendererJSON | string;
type ReactTestRendererNode = ReactTestRendererJSON | ReactText;
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to double-check, but I don’t think the fiber version would ever return a number since it’s always converted to strings inside fiber.


type ReactTestRendererJSON = {|
type : string,
props : {[propName: string] : string },
props : {[propName: string] : any },
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think this is okay. I thought the same thing when grabbing this from the stack implementation. 👍

props: { [propName: string]: string },
children: null | Array<string | ReactTestRendererJSON>,
props: { [propName: string]: any },
children: null | Array<ReactText | ReactTestRendererJSON>,
Copy link
Contributor

Choose a reason for hiding this comment

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

since stack and children behave in slightly different ways I do think that numbers are still numbers at this point, but once again I’ll need to confirm in a short while.

@pedrottimark
Copy link
Contributor Author

Thank you for reviewing. In case it helps you to understand what brought me here: test-rendering dom elements with only a relevant subset of props (given the purpose of that particular test) as the expected value for toMatchObject assertion. You might think of it as an “internal” snapshot whose goal is to minimize irrelevant changes to tests. So, I need to prototype in Fiber also, if possible :)

@pedrottimark
Copy link
Contributor Author

I reverted to string in ReactTestRendererFiber.js because as @iamdustan suggested, the tests confirm a difference between Fiber and Stack reconciler:

Copy link
Contributor

@iamdustan iamdustan left a comment

Choose a reason for hiding this comment

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

I think this looks good @gaearon. Thanks @pedrottimark.

@gaearon gaearon merged commit f56cf66 into facebook:master Jan 23, 2017
@gaearon gaearon added this to the 15-lopri milestone Jan 23, 2017
@pedrottimark pedrottimark deleted the react-test-renderer-json-string branch February 15, 2017 14:52
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.

None yet

4 participants