Restore wrapper auto-update behavior behind a feature flag?

[jwbay]

The breaking change in v3 around needing .update() calls is responsible for the vast majority of our broken tests after upgrading:

right after react@16, enzyme@3 upgrades:
Tests:       1232 failed, 4816 passed, 6048 total

after patching auto-update behavior back into enzyme by editing node_modules:
Tests:       325 failed, 5723 passed, 6048 total

I did see the mention in the migration guide that it was a conscious choice, but I'd disagree with it being an uncommon need. There are two things that drive a high level of need for .update for us -- leveraging component state heavily and making AJAX calls directly in components. The former leads to lots of lifting state up and letting child components change state in parents (<ChildComponent prop={() => this.setState({ x: 1 })} />). The latter leads to async setState calls.

If a project leverages external data stores like Redux, or uses some kind of library that externalizes data fetching like Apollo, everything comes in through props and I would imagine the need for .update likely goes down drastically.

Could we restore the auto-update behavior behind a flag (ideally global 😅)? The implementation is basically just the guts of .update put into .getNodeInternal functions:

if (this[ROOT] === this) {
  this[NODE] = getRootNode(this[RENDERER].getNode());
  this[NODES] = [this[NODE]];
}

[lelandrichardson]

@jwbay unfortunately, I don't think this is really possible (at least not without breaking a LOT more tests). Feel free to try your proposed change in the enzyme repo and see what breaks. I imagine if we tried this on airbnb's test suite the number of breakages would be astronomical.

If this is what's responsible for the bast majority of your broken tests, that's a good thing... it means fixing those breakages is a simple and understandable process?

[Tom910]

Same problem. 200 of 2000 failed files with tests

[jwbay]

@lelandrichardson Thanks for the reply. I was able to get something together at #1186.

I agree that fixing the breakages would be straightforward, but what would bug me more than anything is that we'd have to keep adding more going forward. Referring back to #360 and #490, auto-update behavior is basically just a quality-of-life thing but I feel it makes enzyme's behavior much more intuitive. Anecdotally, I don't miss having to explain to new React devs when .update is needed and when it isn't, and dealing with "is this .update() call necessary?" in code reviews.

For what it's worth, I kind of regret opening this issue with the broken test count -- it wasn't my intention to come off as complaining. I know v3 was a ton of work and it's greatly appreciated. I was just trying to offer a data point against part of the rationale offered in the migration guide for the change.

[seanmheff]

I'm in the same boat. I've updated to V3 and I now have 319 of 6059 tests failing. It seems that most failures are due to the changes to the auto updating.

I'm pretty annoyed at the breaking changes. Yes, I can go and fix the failing tests by adding an explicit update() call. But what about my existing tests that pass with v3. When I refactor components, existing tests may pass by coincidence, since I'm missing an update() call that I may now potentially need. I don't want to go through 1000's of tests and check they still make sense.

I want to have faith in my testing framework. I'm at a loss as to what approach I take from here.

[bookman25]

From a React point of view, there is no concept akin to update(). Every time you inspect state it's after all updates are processed.

With enzyme@3, you can actually test states that are not valid/real world states. There doesn't appear to be a clear delineation about when calling update() is required. As a result developers might miss calling update() and get results that lead to false positives/negatives.

[jcrben]

My problem is not as acute so I did the migration work but I noticed in metabase/metabase#6757 (comment) that @attekei monkey-patched the find method - seems like metabase/metabase@2224bc3#diff-e6f9afd5436578853ededeefeaad6a2dR364 might be the commit.