enzymejs / enzyme

JavaScript Testing utilities for React
https://enzymejs.github.io/enzyme/
MIT License
19.95k stars 2.01k forks source link

thoughts on a more convenient adapter layer #1398

Closed jedwards1211 closed 6 years ago

jedwards1211 commented 6 years ago

I don't know too much about the internal details, but I see in the adapter code that you're converting entire React element trees to an internal representation (which is of course why manually syncing with .update() is now necessary).

But is this really the best approach? Why not just have the adapter layer implement all the methods for traversal and inspection of the real React tree, rather than creating an intermediate representation? That way there would be no need to manually sync. Would this not be possible?

ljharb commented 6 years ago

It's important that we distinguish the enzyme API, from the pieces that are required to vary across React versions. Additionally, the current adapter pattern allows for preact, inferno, react-native, vue, or any jsx-based framework to use enzyme.

jedwards1211 commented 6 years ago

@ljharb no I understand that. But I'm wondering if instead of copying a tree from any of those frameworks into an intermediate representation and then searching/traversing the intermediate representation, it would be possible to traverse the live React tree via methods of a React adapter, the live Preact tree via methods of a Preact adapter, etc?

ljharb commented 6 years ago

That would delegate far too much authority to the adapter; it's also important that tests written to work in enzyme work identically when changing adapters without having to change any of the test code.

In other words, enzyme users never call adapter methods directly; enzyme, not the adapters, is in charge of its API.

jedwards1211 commented 6 years ago

@ljharb I haven't explained myself clearly enough. I'm definitely not advocating that any test code would be dependent upon the adapter.

As far as I understand, when a test calls .find on an enzyme wrapper, it searches through an internal representation created by an adapter, right?

What I'm saying is, imagine if the adapter layer provided methods to get the props of a live component instance, and get its child component instances. Then the enzyme wrapper's .find method could use these methods to traverse and inspect the live tree instead of an internal representation, so test code could use .find successfully without having to .update the internal representation.

ljharb commented 6 years ago

That last part is an intentional design decision in v3; you are supposed to use .update and re-.find every time there’s a change.

jedwards1211 commented 6 years ago

This was a change you guys wanted to make even before it seemed like the easiest way to support multiple versions of React?

jedwards1211 commented 6 years ago

Obtaining an immutable copy of the render tree didn't have to be a breaking change; it could have merely been a new method that creates a dead copy while mount wrappers remain live, giving users both options.

ljharb commented 6 years ago

That's true, it didn't have to be; however, @lelandrichardson considered it a better approach, and so made it intentionally - it won't be revisited.

jedwards1211 commented 6 years ago

I see. I was under the impression that supporting multiple versions of React was the main driver of this change.

The only thing that makes me uneasy about this change is, what if my test passes because I forgot to .update, when I would catch a bug if I .update properly?

ljharb commented 6 years ago

Indeed, it was - making the trees immutable helps support multiple versions of React.

The same thing can happen if you initiate an async rerender, but your test isn't async. There's no way around that bad tests can cause hidden failures :-(