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 componentDidUpdate when updating by setState on v16 #1133

Merged

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Sep 19, 2017

In v16, ShallowRenderer doesn't call componentDidUpdate even updating by setState.
facebook/react#10372
This PR is to fix this.

@koba04 koba04 changed the title [WIP]Fix componentDidUpdate when updating by setState on v16 Fix componentDidUpdate when updating by setState on v16 Sep 19, 2017
@koba04
Copy link
Contributor Author

koba04 commented Sep 19, 2017

l know ShallowRenderer v16 calls cDU without a context argument but I didn't include that because I think it should be fixed separately.

@@ -143,6 +143,7 @@ class ReactThirteenAdapter extends EnzymeAdapter {
let isDOM = false;
let cachedNode = null;
return {
disableComponentDidUpdateOnSetState: true,
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a really awkward way to extend the adapter interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about this?

return {
  options: {
    disableComponentDidUpdateOnSetState: true
  }

I can do this without to extend the adapter interface but it requires to access the React version directly from ShallowWrapper. I think ShallowWrapper should depend on adapters only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if the option should actually be the opposite. enableComponentDidUpdateOnSetState, making it an opt-in flag for adapters, and one that only react 16 will be using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lelandrichardson I think ShallowRenderer won't support componentDidUpdate in future releases so the flag is required by only versions before v16. Using enableComponentDidUpdateOnSetState means that enzyme has to set the flag for future versions of React permanently. So I used disableComponentDidUpdateOnSetState for this.
But it might make sense to use enableComponentDidUpdateOnSetState and set the flag until dropping v15 support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that makes sense. I think there might be more use cases for adding "options" for the adapter. I think i like giving them their own namespace though: options.enableComponentDidUpdateOnSetState or something.

Also, I wonder if it's better to put this on the adapter instance itself instead of the object it returns from createRenderer? This way, we can set an empty this.options = { ... } in the EnzymeAdapter base constructor, which would mean people wouldn't have to set defaults.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! I've updated this PR.

@@ -3403,8 +3403,7 @@ describe('shallow', () => {

context('updating state', () => {
// NOTE: There is a bug in react 16 shallow renderer where prevContext is not passed
// into componentDidUpdate. Skip this test for react 16 only. add back in if it gets fixed.
Copy link
Member

Choose a reason for hiding this comment

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

prevContext should be passed into componentDidUpdate; is react not going to fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. #1139 is a PR for that.
You can see the change at [Unreleased] section in CHANGELOG.md

https://github.com/facebook/react/blob/master/CHANGELOG.md

Copy link
Member

Choose a reason for hiding this comment

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

Then if that's fixed, why do we need an option in the adapter?

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 PR is to support that componentDidUpdate no longer receives prevContext argument, which is not only ShallowRenderer.
This PR is to support that ShallowRenderer no longer call componentDidUpdate.
These are different fixes so I added the options each other.

Copy link
Member

Choose a reason for hiding this comment

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

Why does componentDidUpdate not receive a prevContext argument anymore? It seems like it should.

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 reason seems to be the following. cc: @bvaughn

Even with that parameter present- if context isn't managed in the same way as props and state (PR #8655) then it could be inconsistent between the instance and the fiber.

Update: Chatted in meatspace with @acdlite about this. He pointed out another existing stack limitation with fiber WRT shouldComponentUpdate. (Seems like context is already flaky but fiber will make it more so.) The only safe way to use context is to pass singleton/static props (eg the way react-redux Provider passes down store) that components subscribe to for changes.

facebook/react#8631 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Wow, that seems like a pretty major change to the way context works then.

I've not found context to be flaky at all in React <= 15.

@lelandrichardson any suggestions for a better name for this part of the adapter interface?

Copy link
Contributor

@bvaughn bvaughn Sep 25, 2017

Choose a reason for hiding this comment

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

Edit: My previous comment linked to the same response @koba04 did. Totally not helpful. Sorry. 😄 It's Monday morning.

There are pretty decent notes on facebook/react/pull/8631. The discussion/debate on how to handle prevContext in 16 stretched across 4 months. It was somewhat controversial. But I think the resolution we landed at was the best choice of those that we had. Context will need to change in the future to avoid the potential pitfalls it has always had (and still currently has).

In the meanwhile, this is still good advice:

The only safe way to use context is to pass singleton/static props...that components subscribe to for changes.

@@ -145,6 +145,12 @@ function nodeToHostNode(_node) {
}

class ReactSixteenAdapter extends EnzymeAdapter {
constructor() {
super();
this.options = { ...this.optins,
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

@lelandrichardson lelandrichardson 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 comfortable with this. @ljharb what do you think?

@@ -145,6 +145,12 @@ function nodeToHostNode(_node) {
}

class ReactSixteenAdapter extends EnzymeAdapter {
constructor() {
super();
this.options = { ...this.options,
Copy link
Member

Choose a reason for hiding this comment

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

...this.options needs to go on its own line

@@ -40,4 +40,4 @@
"react": "^15.5.0",
"react-dom": "^15.5.0"
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

all files need to have a trailing newline

const prevState = instance.state;
const prevContext = instance.context;
let shouldRender = true;
// dirty hack: avoid calling shouldComponentUpdate twice
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? can you elaborate in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment. Does this make sense?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM once these updates are done

) {
originalShouldComponentUpdate = instance.shouldComponentUpdate;
instance.shouldComponentUpdate = (...args) => {
shouldRender = originalShouldComponentUpdate.apply(instance, args);
Copy link
Member

Choose a reason for hiding this comment

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

should this function also instance.shouldComponentUpdate = originalShouldComponentUpdate?

I wonder if this should use sinon as a dependency rather than overwriting; what about things like enumerability, setters, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this should use sinon as a dependency rather than overwriting; what about things like enumerability, setters, etc?

Yeah, I can write this like the following if I use sinon.

        let shouldRender = true;
        let spy;
        if (instance.shouldComponentUpdate) {
          spy = sinon.spy(instance, 'shouldComponentUpdate');
        }
        instance.setState(state, callback);
        if (spy) {
          shouldRender = spy.getCall(0).returnValue;
          spy.restore();
        }

Also sinon seems to have a better support about enumerability so I think it's worth to consider this after releasing v3. ES2015 Proxy might be an option for that.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid Proxy; I'm not sure it would work here anyways, and it would very tightly constrict the engines enzyme can run on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. To support < Node v6, sinon.spy would be nice.

Copy link
Member

Choose a reason for hiding this comment

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

A followup PR is always appreciated :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll work on it 👍

@ljharb ljharb force-pushed the fix-component-did-update-on-shallow branch from ea4b71c to d5373a3 Compare September 26, 2017 03:48
@lelandrichardson
Copy link
Collaborator

made small tweak based on @ljharb's comment. merging now in interest of time. Thanks!

@lelandrichardson lelandrichardson merged commit 8cf9661 into enzymejs:master Sep 26, 2017
@koba04
Copy link
Contributor Author

koba04 commented Sep 26, 2017

Thanks!

@austinh
Copy link

austinh commented Oct 10, 2017

Has this been merged into v3.1.0? Because I cannot get componentDidUpdate to run when using setState

@ljharb
Copy link
Member

ljharb commented Oct 10, 2017

Yes, it's in v3.1; please file a new issue if you have trouble still.

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.

5 participants