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

[RFC] Enzyme Adapter + React Standard Tree Proposal #742

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,5 @@
* [Selectors](/docs/api/selector.md)
* [Change Log](/CHANGELOG.md)
* [Future](/docs/future.md)
* [Adapter & Compatibility Proposal](/docs/future/compatibility.md)
* [Contributing Guide](/CONTRIBUTING.md)
5 changes: 5 additions & 0 deletions docs/future.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ There are several things we'd like to address with Enzyme that often get asked.
of projects that we plan on addressing in the near future:


### Enzyme Adapter & Compatibility

[See the full proposal](future/compatibility.md)


#### Improved CSS Selector Support

Currently, "hierarchical" CSS selectors are not supported in Enzyme. That means that the only CSS
Expand Down
191 changes: 191 additions & 0 deletions docs/future/compatibility.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
# Enzyme Adapter & Compatibility Proposal


## Motivation

This proposal is attempting to address a hand full of pain points that Enzyme has been
Copy link
Member

Choose a reason for hiding this comment

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

s/hand full/handful

suspect to for quite a while. This proposal has resulted mostly [#715](https://github.com/airbnb/enzyme/issues/715),
Copy link
Member

Choose a reason for hiding this comment

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

s/suspect/subject (also, no need to put two spaces after a period unless on a typewriter ;-) )

and a resulting discussion among core maintainers of this project.

The desired results of this proposal are the following:

1. Cleaner code, easier maintenance, less bug prone.

By standardizing on a single tree specification, the implementation of Enzyme would no longer have
to take into account the matrix of supported structures and nuanced differences between different
versions of React, as well as to some extent the differences between `mount` and `shallow`.

2. Additional libraries can provide compatible adapters

React API-compatible libraries such as `preact` and `inferno` would be able to provide adapters to Enzyme
for their corresponding libraries, and be able to take full advantage of Enzyme's APIs.

3. Better user experience (ie, bundlers won't complain about missing deps)

Enzyme has had a long-standing issue with static-analysis bundlers such as Webpack and Browserify because
of our usage of internal React APIs. With this change, this would be minimized if not removed entirely,
since these things can be localized into the adapter modules, and users will only install the ones they need.

Additionally, we can even attempt to remove the use of internal react APIs by lobbying for react-maintained packages
such as `react-test-renderer` to utilize the React Standard Tree (RST) format (details below).

4. Standardization and interopability with other tools

If we can agree on the tree format (specified below as "React Standard Tree"), other tools can start to use and
understand this format as well. Standardization is a good thing, and could allow tools to be built that maybe
don't even exist yet.


## Proposal


### React Standard Tree (RST)
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 thought of COM - Component Object Model - but this is probably better :-p

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like subbing React for Component as it isn't tied to React but just the idea of a Component.

Copy link
Member

Choose a reason for hiding this comment

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

CST already means "Concrete Syntax Tree" though (a superset of AST)

Choose a reason for hiding this comment

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

  • Virtual Syntax Tree (VST, already taken)
  • VTree

Copy link
Member

Choose a reason for hiding this comment

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

Technically it's not components, it's elements - Element Something Tree, or Something Element Tree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Expanded" Component Tree? 😉

Copy link
Member

Choose a reason for hiding this comment

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

@gaearon yes but those aren't components anymore - a component is what you use to create that plain object (the react element).

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! I mean that those are neither components nor elements really. Colloquially we often say "component tree" though. "Element tree" sounds more technical to me, and misleading for this reason: element tree is exactly what is being returned from render. Anyway, I don't really care.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point too - I'd rather invent a new name than pick a name that isn't accurate, or could be misleading. lots of 🚲 🏠 on this one, for sure.

"Concrete Render Tree"? i dunno

Copy link
Contributor

Choose a reason for hiding this comment

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

RAT: Reactish Abstract Tree.

It's a tree of abstraction over the elements produced by reactish environments


This proposal hinges on a standard tree specification. Keep in mind that this tree needs to account for more
than what is currently satisfied by the output of something like `react-test-renderer`, which is currently
only outputting the "host" nodes (ie, HTML elements). We need a tree format that allows for expressing a full
react component tree, including composite components.

```js
// Strings and Numbers are rendered as literals.
type LiteralValue = string | number

// A "node" in an RST is either a LiteralValue, or an RSTNode
type Node = LiteralValue | RSTNode

// if node.type
type RenderedNode = RSTNode | [Node]

type SourceLocation = {|
fileName: string
lineNumber: number
|}

// An RSTNode has this specific shape
type RSTNode = {|

Choose a reason for hiding this comment

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

suggest adding nodeType to the shape that can be used to differentiate between non-element nodes like text nodes and fragment nodes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nodeType is specific to DOM, and something i'd prefer not to have in this proposal. I'm not exactly sure it would gain us anything either, as strings are represented in this tree as string literals, not nodes.

Choose a reason for hiding this comment

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

@lelandrichardson something like nodeType is not necessarily specific to the DOM. Inferno uses flags to represent something similar for better performance inline with monomorphism.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. It's possible i'm not quite understanding what you mean then. What would the nodeType be for composite components? What about for React Native? Keep in mind that we aren't necessarily concerned about performance as much as we are about the spec being general enough to cover the different libraries/versions/platforms, simple enough to make it easy to implement adapters. nodeType to me seems like it would have a lot of overlap with the information that type provides...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, thanks for weighing in! I hope i'm not dismissing a good idea too quickly!

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I think i see what you are saying now @thysultan , your link to the MDN docs was throwing me off at first. I think this could be a reasonable way to define RSTNode as a disjoint union type instead of multiple types. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would all possible NodeTypes be defined by the spec? Things like Coroutines are very specific to React right now and defining that in the spec would feel like we're coupling it too heavily to one implementation.

Choose a reason for hiding this comment

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

I suppose all possible generic nodes could be defined or at least the possibility to add new nodes could be left open with a dedicated flag, and any library that implements such a nodeType can opt in to use them to define a node. i.e fragments, not all libs implement them but it's a generic enough node that could be used to define a possible node representation, the same could be said for Coroutines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aweary I agree. I'm not sure how I feel about the node types that don't seem to apply to enzyme... but I need to understand them more. If they become a part of react and enzyme needs to know about them in order to provide useful testing APIs, we might still need them.

// Either a string or a function. A string is considered a "host" node, and
// a function would be a composite component. It would be the component constructor or
// an SFC in the case of a function.
type: string | function;

// Whether or not this node is a "Host" node.
host: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

For supporting coroutines and portals, you might want to add a nodeType after all (we call it tag internally in Fiber). It lets you keep track of future React changes. It's fine if it's just 'host' | 'class' | 'function' for now, and adds 'portal' | 'coroutine' at some point later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is good feedback, thanks. What do you think about also having a literal nodeType that wraps things like numbers/strings/etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Fiber, we only have string literals at the renderer level, numbers get converted to strings before that.

I don't see a benefit in wrapping all literals into nodes. Let's just always use strings. Technically we do create fibers for literals when there are several of them but it's an implementation detail and depends on the renderer. For example, DOM renderer inlines a single text child, but test renderer may choose not to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, regarding the above comment: I meant you probably want to replace host: boolean with nodeType/tag: union. Since that covers host: boolean already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gaearon understood. thanks. I'll make some updates to use nodeType.


// The props object passed to the node, which will include `children` in its raw form,
Copy link
Member

Choose a reason for hiding this comment

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

what about the context being passed to the node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hesitate to provide any sort of standardization around context, a feature that is very likely to change, and seems more of a react-implementation-specific thing.

I think enzyme will continue to provide the ability to utilize context through the renderer options (which will be shallow/mount options passed through to the renderer), but we shouldn't make it part of this proposal. Enzyme will continue using the instance in order to fulfill the behavior of these properties, just as it is doing with state and setState

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 it likely to change? Do Fiber, Inferno, or Preact not implement it?

Copy link
Member

Choose a reason for hiding this comment

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

Also, since v13, 14, and 15 all use context, I don't think we can just pretend it doesn't exist. Even if v16 removed it (which I doubt) we'd still need this spec to support it.

Copy link
Contributor

Choose a reason for hiding this comment

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

its often mentioned that the context was a feature implemented kinda in a spur of the moment and is not ideal, however it will stay as is until a better alternative is invented

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, context support was added to React fiber. I don't think they're going away anytime soon.

Copy link
Member

Choose a reason for hiding this comment

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

If I write a component that relies on context, and the tests pass for React 15, I should be able to switch the AST generator to, say, inferno, and have it work identically (assuming that inferno has the concept of "context", in this hypothetical). If context isn't in this spec, how can that be achieved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as this spec is currently written, both context and state are things that will "just work" so long as the libraries implement their public facing API the same. I'm not really seeing these two concepts as belonging in this spec, as they are not relevant to traversal. Even most props don't really matter to this proposal... the only reason props are here is because we use them for traversal. We don't use context or state for any such things.

I think we shouldn't add things to this spec that aren't needed?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I wasn't under the impression that this spec was just for traversal - I was thinking it would be a way to render the entire tree with 100% fidelity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think if we want to iron down things about context, that we can do so by specifying it in the ComponentInstance type and the EnzymeRenderer API.... but I don't think it needs to be part of the tree structure outside of that.

I could see more APIs being put on EnzymeRenderer like provideContextForNode and setStateForNode or something like that, but I was trying to make the API as minimal as possible, and saw that as potentially unneeded.

I think the tree structure (ie, RSTNode) is really just about traversal though.

// exactly as it was passed to the component.
props: object;

// The backing instance to the node. Can be null in the case of "host" nodes and SFCs.
// Enzyme will expect instances to have the _public interface_ of a React Component, as would
// be expected in the corresponding React release returned by `getTargetVersion` of the
// renderer. Alternative React libraries can choose to provide an object here that implements
// the same interface, and Enzyme functionality that uses this will continue to work (An example
// of this would be the `setState()` prototype method).
instance: ComponentInstance?;

// For a given node, this corresponds roughly to the result of the `render` function with the
// provided props, but transformed into an RST. For "host" nodes, this will always be `null` or
// an Array. For "composite" nodes, this will always be `null` or an `RSTNode`.
rendered: RenderedNode?;

// an optional property with source information (useful in debug messages) that would be provided
// by this babel transform: https://babeljs.io/docs/plugins/transform-react-jsx-source/
__source?: SourceLocation;
|}
```

Thoughts:

- If an `RSTNode` has `host: true`, `rendered` will always be an `Array` or `null`. (though this
might change with Fiber I guess).
- `rendered` is a better property name than `children`, as the meaning of `children` is ambiguous
here since it is defined on props/elements etc.
- If a node has `host: true`, loosely speaking that means that an instance of that component
Copy link
Collaborator

@aweary aweary Jan 24, 2017

Choose a reason for hiding this comment

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

I think this thought got cut off?

- For "host" nodes, can `instance` ever be non-null? Could it be the DOM element? Yes/No?


### Enzyme Adapter Protocol

**Definitions:**

An `Element` is considered to be whatever data structure is returned by the JSX pragma being used. In the
react case, this would be the data structure returned from `React.createElement`


```js
type RendererOptions = {
// An optional predicate function that takes in an `Element` and returns
// whether or not the underlying Renderer should treat it as a "Host" node
// or not. This function should only be called with elements that are
// not required to be considered "host" nodes (ie, with a string `type`),
// so the default implementation of `isHost` is just a function that returns
// false.
?isHost(Element): boolean;
}

type EnzymeAdapter = {
// This is a method that will return a semver version string for the _react_ version that
// it expects enzyme to target. This will allow enzyme to know what to expect in the `instance`
// that it finds on an RSTNode, as well as intelligently toggle behavior across react versions
// etc. For react adapters, this will likely just be `() => React.Version`, but for other
// adapters for libraries like inferno or preact, it will allow those libraries to specify
// a version of the API that they are committing to.
getTargetApiVersion(): string;

// Provided a bag of options, return an `EnzymeRenderer`. Some options can be implementation
// specific, like `attach` etc. for React, but not part of this interface explicitly.
createRenderer(?options: RendererOptions): EnzymeRenderer;

// converts an RSTNode to the corresponding JSX Pragma Element. This will be needed
// in order to implement the `Wrapper.mount()` and `Wrapper.shallow()` methods, but should
// be pretty straightforward for people to implement.
nodeToElement(RSTNode): Element;
}

type EnzymeRenderer = {
// both initial render and updates for the renderer.
render(Element): void;

// retrieve a frozen-in-time copy of the RST.
getNode(): RSTNode?;
}
```


### Using different adapters with Enzyme

At the top level, Enzyme would expose a `configure` method, which would allow for an `adapter`
option to be specified and globally configure Enzyme's adapter preference:

```js
import Enzyme from 'enzyme';
import ThirdPartyEnzymeAdapter from 'third-party-enzyme-adapter';

Enzyme.configure({ adapter: ThirdPartyEnzymeAdapter });
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we did a "magical" approach similar to babel, where we look for a node module with a matching standardized prefix and autoloaded that. So say the standard was enzyme-adapater-* and we find enzyme-adapter-react we use that internally.

With that we'd have to have a way to handle multiple enzyme-adapters being found in node_modules. Maybe also have this manual api but first try and do it automatically?

Just thinking about lessening burden and confusion to users. I find most users don't understand how to set up a testing engine and just want to write their tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the explicit API is best, I'm not a huge fan of "magical" auto-configuration approaches like that. What happens if they update from React 14 to 15 and forget to remove enzyme-adapter-react-14 from their dependencies?

Maybe that could be a long-term goal once we're more familiar with actual use patterns.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm more partial to the explicit API as well. It also makes it really clear how to use different adapters in the same test suite. Though I am kind of bummed we will be joining the ranks of tools that don't "just work" with one npm install :/

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer the explicit API.


```

Additionally, each wrapper Enzyme exposes will allow for an overriding `adapter` option that will use a
given adapter for just that wrapper:

```jsx
import { shallow } from 'enzyme';
import ThirdPartyEnzymeAdapter from 'third-party-enzyme-adapter';

shallow(<Foo />, { adapter: ThirdPartyEnzymeAdapter });
```

By default, Enzyme will ship with adapters for all major versions of React since React 0.13:
Copy link
Member

Choose a reason for hiding this comment

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

if React 16 comes out before this is done, then we'd probably support 0.14 - 16 instead?

Perhaps more accurate to state "the latest, and previous two, major versions of React"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm actually maybe rethinking this. Perhaps enzyme shouldn't ship with any adapter? Those are all added via a second dependency and import?

As for supporting react 0.13. Structuring the project this way should make supporting react 0.13 much much easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think adapters should not be shipped automatically. For users of inferno/preact it becomes bloat. Even for users of React on a specific version, it becomes bloat getting adapters for other versions.

I'm thinking we have a specific react-enzyme-adapter that matches react versions. I don't think we should have different repos for each version of react.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm fine not shipping any adapter directly, but we'd have to be very very clear with instructions and error messages so people know how to add one.


```js
import React13Adapter from 'enzyme-adapter-react-13';
import React14Adapter from 'enzyme-adapter-react-14';
import React15Adapter from 'enzyme-adapter-react-15';
// ...
```

### Validation

Enzyme will provide an `validate(node): Error?` method that will traverse down a provided `RSTNode` and
return an `Error` if any deviations from the spec are encountered, and `null` otherwise. This will
provide a way for implementors of the adapters to determine whether or not they are in compliance or not.
Loading