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

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.

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?

@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.

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?

That makes sense! I've updated this PR.

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

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

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

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.

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

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)

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?

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.

typo

Fixed

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

all files need to have a trailing newline

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

Added a comment. Does this make sense?

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?

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.

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.

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

A followup PR is always appreciated :-D

I'll work on it 👍