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

feat: export container from render #567

Merged
merged 13 commits into from
Oct 21, 2020
Merged

Conversation

klekowskim
Copy link
Contributor

@klekowskim klekowskim commented Oct 6, 2020

Summary

Exports root test instance from render method. It is a change mentioned in this feature request: #566

After this change it is possible to get test instance from render method:

const { container } = render(<View />);

@klekowskim
Copy link
Contributor Author

I can also change it to export container, instead of the root. To do that probably I would need to add internal wrapper, as it was in the second library here.

In that case the API would be more like in the react-testing-library.

@mikeduminy
Copy link
Contributor

+1 to this change, and +1 on adding container and baseElement

@mdjastrzebski
Copy link
Member

I guess I am in favor of exposing container instead of root. Getting root element from contains is very easy and we have maintain api between RTL and RNTL with all benefits of such (code and knowledge sharing, etc).

Regarding wrapping of root element, I think that wrapping it in plain View should do the cases TL/RN v6 did wrap it in AppContainer from ''react-native/Libraries/ReactNative/AppContainer' but reading the code of AppContainer this seems not useful as it deals with low-level RN stuff like profiling, logbox, inspector, etc.

@klekowskim
Copy link
Contributor Author

@thymikee What do you think? I can adjust it but I'd like to know if it is going to be merged 😄

@thymikee
Copy link
Member

Let's do it. Like @mdjastrzebski said

src/render.js Outdated Show resolved Hide resolved
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Removed the default wrapper, because it's orthogonal to returning the root instance and would require us to release this feature in a major release.
I left the peer dep on react-native, because it should be there anyway.

Thanks for the contribution!

@thymikee thymikee changed the title feat: export root test instance from render feat: export container from render Oct 20, 2020
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Overall looks good, consider adding testID/nativeID checks in tests

src/__tests__/render.test.js Outdated Show resolved Hide resolved
src/__tests__/render.test.js Outdated Show resolved Hide resolved
@thymikee thymikee merged commit 78fb456 into callstack:master Oct 21, 2020
jason0x43 added a commit to jason0x43/react-native-testing-library that referenced this pull request Oct 28, 2020
Update the TS types for the new `container` property that was added to
the output of `render` in callstack#567.
thymikee pushed a commit that referenced this pull request Oct 28, 2020
Update the TS types for the new `container` property that was added to
the output of `render` in #567.
@pvinis
Copy link

pvinis commented Nov 9, 2020

I just tried to use this and saw a couple of issues:

  • typings/index.d.ts is not updated to contain the container.
  • ReactTestInstance is not exported in typings/index.d.ts so I can't add this as a type to my helper test function
  • I need access to ErrorWithStack and getNodeByText. Is there a way to use these on container?

@pvinis
Copy link

pvinis commented Feb 10, 2021

@thymikee are these 👆 done or being worked on, or would it be better if I make an issue for these?

@thymikee
Copy link
Member

I don't think these are addressed. Please create an issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants