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

Support compatible component libraries with defined spec #715

Closed
trueadm opened this issue Dec 2, 2016 · 34 comments
Closed

Support compatible component libraries with defined spec #715

trueadm opened this issue Dec 2, 2016 · 34 comments

Comments

@trueadm
Copy link

trueadm commented Dec 2, 2016

I was thinking, given that Inferno and Preact both aim to support the React API – would it be in the interest of this project to add a layer to enable support of alternative rendering engines other than React? Both libraries offer compatibility packages, i.e. inferno-compat and preact-compat that aim to provide backwards support for React via alias changes.

@blainekasten
Copy link
Contributor

I don't know much about Inferno, but Preact is gaining a lot of popularity. I'm not sure how well it would work as enzyme dives deep into unsupported React internals. So if it's possible with something like preact-compat to do I think it would be worth considering. Though if it adds too much weight to maintain both interops it's likely not worth it..

I'd be curious on hearing from @ljharb

@trueadm
Copy link
Author

trueadm commented Dec 2, 2016

@blainekasten Inferno and Preact have similar goals – if you have time, please also look into Inferno. I know the author of Preact well and I'm certain @developit would love this to be landed if possible.

In terms of using React internals, that will soon have to change as React pulls away internal access using internal APIs (even more so with the upcoming React Fibre) so maybe now would be a good time to look how these things can overlap and help one another.

@aweary
Copy link
Collaborator

aweary commented Dec 2, 2016

I've talked with @developit briefly about supporting Preact. I think initially any interop would be supported via those *-compat packages, which would need to support ReactTestUtils and the various internal APIs we use.

Long term it would make more sense for Enzyme to expose a way for consumers to configure the rendering engine, instead of supporting Inferno and Preact directly. This might mean you'd need to provide modules like inferno-shallow-renderer, for example.

@developit
Copy link

I would love to have support for this and I'm willing to help same as @trueadm.

@aweary - how plausible do you think it would be to make the shallow/full/static renderers pluggable/injectable?

@aweary
Copy link
Collaborator

aweary commented Dec 2, 2016

I think it's pretty plausible, a lot of what we pull from React is isolated in src/react-compat.js, which is where we require in most (all?) the React packages we consume. If we can decide on a config API we could possibly read it from there and require user defined packages instead that export the same APIs.

That would take care of a sizeable portion of the work needed to support this level of interop, though its likely not as easy as it sounds.

Ultimately it depends on whether this is even something we can agree is a goal for Enzyme. @ljharb would be the authority on that.

@ljharb
Copy link
Member

ljharb commented Dec 2, 2016

I'm not familiar with either, but in general enzyme is for testing react, not testing jsx - if something "supports react" then enzyme should work just fine with it - in other words, your component snd it's API should be independent of your rendering system. When you use enzyme you're not meant to test React itself, React's rendering is used to test your own code - as such, what would be the value in having enzyme use an alternative rendering engine?

@developit
Copy link

Both libs are API-compatible with React (they implement the same semantics and component model), but do not support the myriad of internal hooks React exposes under lib/.

As for reasoning, for me the concern would be: rendering tests with React but then using an entirely different library in production would be a risk (no assured parity), and it would mean having React installed as a dependency just to run Enzyme tests.

If it's not a goal for Enzyme to support react-like libraries that's fine, it just means we're going to end up with Enzyme clones for these libraries with the same outward API but differing renderers - probably a less elegant solution to the issue.

@trueadm
Copy link
Author

trueadm commented Dec 2, 2016

@ljharb maybe the issue is you haven't actually familiarised yourself with either? Both libraries are React-like, in that they follow the same public API, but they differ on how their internals work, and this includes their private API (which the React team are looking to change in the future, as you really shouldn't be using it) under lib.

... and @developit just posted the exact same reply as this so I'm going to say the same thing if I continue :)

@ljharb
Copy link
Member

ljharb commented Dec 2, 2016

We already have a lot of complexity in enzyme to manage multiple react versions - I'm all for configurability but adding maintenance cost for the sake of any libraries without a clear critical mass of usage (I'd barely even classify Riot as having enough usage) seems problematic.

That said, I'm happy to continue exploring it - but the solution shouldn't be "more complex branching in react-compact.js"

@developit
Copy link

Not to play the stats game, but given that Riot is not even the same API as React it seems an odd reference.

I'm curious if you had any ideas around where one might hook into Enzyme to make the three rendering modes pluggable. Emulating react's internals is certainly an option, but it's quite ugly to do (lots of mocking and proxies), I'd be interested to know if there was a place in Enzyme to handle each mode's rendering. I'm nowhere near familiar enough to think of the real surface area, but in my head this seems like:

render(Element, VNode) => Element
shallowRender(Element, VNode) => Element
staticRender(VNode) => String

@aweary
Copy link
Collaborator

aweary commented Dec 2, 2016

We already have a lot of complexity in enzyme to manage multiple react versions - I'm all for configurability but adding maintenance cost for the sake of any libraries without a clear critical mass of usage (I'd barely even classify Riot as having enough usage) seems problematic.

I'm sure @developit can speak more to this, but Preact in particular has been gaining a lot of traction, especially in the progressive web app community. And it wouldn't be for the sake of any one library; React's component model has influenced (and will influence) other frameworks, and Enzyme could position itself as a testing utility for any one of those frameworks willing to provide a compatibility layer.

That said, I'm happy to continue exploring it - but the solution shouldn't be "more complex branching in react-compact.js"

The idea isn't to add more branching for every library we could possibly support. The idea is that, if the user has provided their own config (which provides a matching API) for how to render/parse their components, then we use that and move on. It's up to the library authors to provide packages that work correctly.

@trueadm
Copy link
Author

trueadm commented Dec 2, 2016

@ljharb I'm not sure why you bring popularity metrics into this – Inferno and Preact are never going to get the popularity of React. We're addressing a different and yet important problems in the webdev world and that is to do with performance and size – especially in regards to mobile users.

Anyway: we're just looking for a clean way to hook into Enzyme so we can help improve the experience for users of our libraries when it comes to testing. So if there was a way to hook into the main API calls for Enzyme and supply library specific helpers for things like Component, createElement for when they call shallowRender, render etc that would be amazing.

@developit
Copy link

Couldn't have said it better (both of you @trueadm @aweary). If there is a way to make Enzyme pluggable, the complexity is moved out of core and into plugins. To me that would be a huge win and also be a boon for the utility and longevity of Enzyme. I think you'd see Vue and Riot plugins pop up rather quickly.

@ljharb
Copy link
Member

ljharb commented Dec 2, 2016

@developit it was just the first alt-React that popped into my head that I'd heard of.

Note I'm not playing a "stats game", i'm saying that complexity has costs, and "userbase size" is one of the things that needs to be weighed in that discussion.

If we can come up with a way to make enzyme configurable in these ways - especially if it can make the current react-compat code simpler, and/or if it can ameliorate the "implicit optional dependency" problem we have now - that would be a great thing.

I don't think we need to "emulate React's internals" necessarily - a likely better alternative is coming up with the API we actually want - and then writing adapters for it to each of the versions of React, and for any alt-React that wants to work with enzyme. I'm thinking we'd have enzyme-react-13, enzyme-react-14, enzyme-react-15, etc - and that way all the dependencies for that version of react could be removed from enzyme and placed in those kinds of packages.

@aweary
Copy link
Collaborator

aweary commented Dec 2, 2016

I'm thinking we'd have enzyme-react-13, enzyme-react-14, enzyme-react-15, etc - and that way all the dependencies for that version of react could be removed from enzyme and placed in those kinds of packages.

👍 I really the direction of this idea. It makes enzyme cleaner and configurable. What would the API those packages expose look like?

@ljharb
Copy link
Member

ljharb commented Dec 2, 2016

That's what we'd want input from everyone on - the React team, as well as the Preact/Inferno/etc devs, to come up with the most future-facing API surface.

@aweary
Copy link
Collaborator

aweary commented Dec 2, 2016

First thoughts:

  1. Full, shallow, and static renderers (mount, shallow, render)
  2. Component tree traversal (a standard API like that of bill might be nice)
  3. Getting state or props from an instance
  4. Settings state or props
  5. Accessing an instance's DOM node
  6. Text content
  7. Event simulation (like ReactTestUtils.Simulate)

cc @gaearon I know you guys are busy with Fiber, but any input you may have would be appreciated.

@blainekasten
Copy link
Contributor

This is a big set of exciting changes. It might be worth having a sort of google hangout to flesh out everything that is necessary from the different sides.

@ljharb
Copy link
Member

ljharb commented Dec 2, 2016

(in addition to setting state/props, a way to force a rerender)

@bvaughn
Copy link
Contributor

bvaughn commented Dec 3, 2016

I'm not sure how well it would work as enzyme dives deep into unsupported React internals.

This is going to cause problems once the fiber reconciler lands. For example, these lines reference a property _renderedComponent that no longer exists in the fiber reconciler. (I realize this is only tangentially related to the topic at hand, but...decoupling from internals is probably a great goal.)

@developit
Copy link

Yeah the timing here might actually be pretty good if you guys are looking to support both React 15.x and the new stuff.

@cpojer
Copy link
Contributor

cpojer commented Dec 3, 2016

The aim of this, as I understand it, is to make enzyme a generic component testing library, which I think is fantastic. Maybe it can even be rebranded to that so that in the future we won't get any more people asking whether they should use Jest or Enzyme (use both, dammit! :D ).

I'm just jumping in here to broaden the discussion a little bit. I think we should remove ourselves a bit from frameworks (both for testing and rendering) and think of the high level problem that we are all trying to solve: how do you test your UI component? We have a ton of different solutions to that nowadays but it often boils down to:

  • Smoke test: Run the code in the first place, which for UI tests wasn't something we even used to do.
  • Interact with the component and assert state changes happen.

We've also invented a bunch of different rendering modes:

  • shallow render
  • deep rendering
  • arbitrary levels of rendering through mocking out components, usually at the module boundaries

And there are a bunch of different renderers right now that are used for testing React code:

  • react-dom through enzyme (mostly? for web components)
  • react-test-renderer (both for web and RN components)

And then we have two major ways to assert what we are doing is correct:

  • assertions
  • snapshot testing

That's a ton of stuff, but when I think about all of this together and how enzyme fits into this, I would suggest this:

  • Rebuild enzyme on top of react-test-renderer and extend the test-renderer's API with necessary methods like find(MyButton). Maybe enzyme could even take over react-test-renderer. Drop the use of react-dom entirely.
  • Once enzyme consumes the output of react-test-renderer, it should be easy to enable testing for other frameworks if they build a test-renderer for their project, like preact/inferno.
  • We end up putting enzyme as a middle layer between an arbitrary framework + their renderer and how we make sure our tests are correct.

To sum up, it would look something like this:

react   > react-test-renderer   +          shallow                                                   
inferno > inferno-test-renderer + enzyme > deep render > enzyme interact > assertions / snapshots
...     > …                     +          arbitrary                                     

This would also make it so that enzyme will automatically work for any codebase that normally uses other renderers. Together with Jest's powerful mocking for native components (like in React Native), this could be a pretty powerful testing tool that captures the entire component-based framework world.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

Emulating react's internals is certainly an option, but it's quite ugly to do (lots of mocking and proxies)

I’d also add that React internals themselves are changing in future versions very significantly.

@blainekasten
Copy link
Contributor

This is an incredible conversation. So happy to see input from vendors of all aspects of the ecosystem. It really just speaks to the importance of this library. I think if we're honest, it's pretty difficult coordinating larger changes for this library among the team for different reasons, and to take on this large of a change will require some intense coordination.

I'd love to setup a few meetings.

  1. Meeting with the core team to figure out how we can coordinate this among ourselves.
  • How existing PRs will be included
  • How much time we individually have
  • etc
  1. A meeting with interested parties
  • Ensure we design the API to work for everyone's use case
  • See if we can count on dev work from anyone else
  • Get additional input for anything we've missed

If you are interested in being part of that meeting, give this comment a 👍 reaction. Or if you think this is unnecessary, give it a 👎 reaction :) The holidays will definitely slow this process down. But would love to aim to get the first meeting done before the holidays, and the second after the new year. Could be sooner if it all aligns.

@blainekasten
Copy link
Contributor

🍶 Core Meeting 🍶

We just had our first core meeting in regards to this change. Lots of exciting discussion happened and we're starting to move forward with this. More details to come, but I wanted to give a high level overview of our plan moving forward.

Spec

The direction we think makes the most sense right now is to define an AST spec for Enzyme to consume. With defining a spec, we can break free of internals of all component libraries and be truly agnostic to the components themselves. We're going to start on this right away. We'll be working on it for some time until we feel semi-confident in our approach (It'll be worked on publicly in a branch on this repo).

Once we reach that point, we plan to reach out to as many component library authors as possible to get feedback on the spec; ensure it covers every case and will be compatible with as many component libraries as possible.

Spec Building Libraries

After a spec is properly defined, we can transition into creating compatibility libraries. These libraries would be aware of the internals of the component library in a way to generate a testing AST which Enzyme will consume.

Examples:

// CONCEPT: preact-example
const PreactComponent = <div><span /></div>;

shallow(generateAst(PreactComponent));

We think this should support everyone and also set Enzyme up for a more stable future. We'll give more updates when we've started work on the AST RFC. Thanks to everyone who's contributed thus far to get this ball rolling! 🎉 🎉 🎉

@ljharb
Copy link
Member

ljharb commented Dec 20, 2016

Another variant is letting enzyme be configured via a stateful method to use a given AST generator - ie:

import { setASTGenerator } from 'enzyme';
import generateAST from 'preact-enzyme-ast'; 
setASTGenerator(generateAST);

and then test code itself wouldn't have to change to switch renderers, only the bootstrapping code.

@blainekasten
Copy link
Contributor

We should call it an ACT: Abstract Component Tree 😎

@aweary
Copy link
Collaborator

aweary commented Dec 20, 2016

We should call it an ACT: Abstract Component Tree 😎

Then we could write a guide on building your own and call it "Get your ACT together"

@developit
Copy link

yes that won't be confusing at all 😂

@ljharb
Copy link
Member

ljharb commented Dec 20, 2016

and then we could name the repo that houses the spec and tests for it reACT!

@cpojer
Copy link
Contributor

cpojer commented Dec 20, 2016

does your plan include moving to react-test-renderer and building a shallow rendering feature for the test renderer? That will enable us to use enzyme together with Jest and react-native.

@aweary
Copy link
Collaborator

aweary commented Dec 20, 2016

@cpojer we briefly discussed something along the lines of react-test-renderer providing an interface for whatever common AST/ACT structure we're hoping to define, and then being able to consume that. That would require buy-in and cooperation from the React team as well.

@cpojer
Copy link
Contributor

cpojer commented Dec 20, 2016

I think the React team would be happy to find an owner for the react-test-renderer but yeah, I'm not sure what exactly that would look like. Me personally, I think using enzyme as a layer in between react-test-renderer and Jest seems pretty much perfect. We can provide the mocks at the module boundary for native components; that way you can use enzyme for components written for any render target and less dependent on react-dom.

@blainekasten blainekasten changed the title Support for Inferno and Preact? Support compatible component libraries with defined spec Dec 20, 2016
@lelandrichardson
Copy link
Collaborator

Closing this now. Please continue the conversation in #742

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants