-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Docs] Update docs around SFC instances in React 16 #1243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! So far just a few comments.
docs/api/ShallowWrapper/instance.md
Outdated
@@ -2,20 +2,62 @@ | |||
|
|||
Gets the instance of the component being rendered as the root node passed into `shallow()`. | |||
|
|||
NOTE: can only be called on a wrapper instance that is also the root instance. | |||
NOTE: can only be called on a wrapper instance that is also the root instance. With React `16.x.x`, `instance()` returns `null` for stateless React component. See example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/stateless React component/stateless functional components
, s/example./example:
docs/api/ShallowWrapper/instance.md
Outdated
|
||
#### Returns (React 16.x.x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the .x.x
anywhere; 16.x
should be sufficient (same throughout)
docs/api/ShallowWrapper/instance.md
Outdated
|
||
- `ReactComponent`: The Statefull React component instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/statefull/stateful/g
@ljharb PR was updated. Please let me know in case of mismatches. |
docs/api/ShallowWrapper/instance.md
Outdated
|
||
expect(instance).not.toBe(null); // This will fail, because instance is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be jest matcher syntax; we use chai's expect. could you update it to be expect(instance).not.to.equal(null)
instead? (same throughout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change.
docs/api/ShallowWrapper/instance.md
Outdated
const wrapper = shallow(<Stateful />); | ||
const instance = wrapper.instance(); | ||
|
||
expect(instance).not.toBe(null); // This will pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's preserve the original assertion here too: expect(inst).to.be.instanceOf(MyComponent);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an original example it's not clear (as for me) what is <MyComponent />
. I mean, is it stateful or stateless. And whole purpose of this update is to show this specific case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to show "null or not null"; I'm asking not to delete the "instance of MyComponent" checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I did not catch it.
@ljharb PR was updated. |
docs/api/ShallowWrapper/instance.md
Outdated
|
||
expect(instance).not.to.equal(null); // This will pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add here expect(instance).to.be.instanceOf(Stateful);
as well
docs/api/ShallowWrapper/instance.md
Outdated
const wrapper = shallow(<Stateless />); | ||
const instance = wrapper.instance(); | ||
|
||
expect(instance).not.to.equal(null); // This will pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add here expect(instance).to.be.instanceOf(Stateless);
as well
docs/api/ShallowWrapper/instance.md
Outdated
const wrapper = shallow(<Stateful />); | ||
const instance = wrapper.instance(); | ||
|
||
expect(instance).not.to.equal(null); // This will pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add here expect(instance).to.be.instanceOf(Stateful);
as well
docs/api/ShallowWrapper/instance.md
Outdated
|
||
expect(instance).not.to.equal(null); // This will fail, because instance is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add here expect(instance).not.to.be.instanceOf(Stateless);
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good!
Is this something we can merge master into and get approved? Was quite helpful for me and was annoying to have to dig through PRs to find it. |
.instance()
method
Related to #1232