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

Conversation

lelandrichardson
Copy link
Collaborator

to: @ljharb @aweary @nfcampos @blainekasten
cc: @cpojer @sebmarkbage @spicyj @developit @trueadm @bvaughn

This is a proposal and I was hoping to get some feedback from people in this PR. The enzyme maintainers met to discuss #715 and what we can do to support this and also improve on the maintainability and usefulness of enzyme.

I'm looking for any and all feedback. Thanks!

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.

```
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a ```js or something so we get some highlighting?

@lelandrichardson
Copy link
Collaborator Author

@cpojer FYI, I have a branch of react that makes react-test-renderer return something kind of similar to what i'm proposing here. I m happy refactor it to actually implement this spec if we agree on it.

@cpojer
Copy link
Contributor

cpojer commented Dec 20, 2016

@lelandrichardson please consider the downstream effects of a change like this; if you break pretty-format, you will break every single snapshot test that's out there, so we need to make sure we have a proper upgrade path rather than making all tests fail on a react update with no new version of Jest or something. pretty-format is here: https://github.com/facebook/jest/tree/master/packages/pretty-format

@lelandrichardson
Copy link
Collaborator Author

@cpojer definitely. I was actually envisioning it as a separate method (ie, toRSTNode() or something in addition to toJSON.

We aren't losing any information here, only gaining, so it wouldn't be hard to create a function that transformed an RSTNode to the current "JSON" value that react-test-renderer returns. That way, we could just implement the renderer once, but refactor toJSON() to return what it's currently returning.

I'm certain i'm not considering everything, but that's the kind of feedback I was hoping to get in this thread as well.

## 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

// an SFC in the case of a function.
type: string | function

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

```js
type ComponentInstance = {
state: object?
context: object?
Copy link
Member

Choose a reason for hiding this comment

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

context seems like it should be alongside props, not alongside state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

technically, props is also here, i'm just not listing it as we won't be using it directly. I'm actually tempted to just completely remove context here in this proposal

type Node = LiteralValue | RSTNode

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

@cpojer
Copy link
Contributor

cpojer commented Dec 20, 2016

yeah that's great, as long as enzyme objects at the end also provide a toJSON feature the get back that same React tree so it can be passed on to the pretty-printer, I think this is all sound from my point of view.

// it's an array of Nodes, meaning that they are LiteralNodes or RSTNodes. This means that
// non-rendering values such as `false`, `null`, and `[]` wouldn't make it in here. This is also
// a "flat" array.
children: [Node]?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why we need to duplicate information that’s always derivable from the props? Is this a convenience?

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 because we don't have the knowledge of how to transform an Element to a Node. That's the responsibility of the adapter.

RSTNode.children !== RSTNode.props.children. The props.children will be completely untouched (it could be anything, really). This also abstracts away the flattening etc. that react would do.

Copy link
Contributor

@gaearon gaearon Dec 20, 2016

Choose a reason for hiding this comment

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

If I have <div><Foo /></div>, and <Foo /> renders <Bar />, what's going to be in children of div?

If it's truly a "transformed version of props.children", then it will be shallow Foo and which itself won't link to Bar RST node. Because props.children is shallow and does not contain rendering result.

However this doesn't make sense to me because per next comment, rendered is null for hosts. So we end up not having any way to traverse from div to Bar.

Coming back to this, it seems like children is not merely a "transformed version of props.children", but actual RST nodes corresponding to them. You can't write a "helper" that transforms props.children to these children because it's the rendering algorithm itself: you'd have to descend down. So I don't see why separate children from rendered. It's the same concept: an array of children nodes.

Imagine React DevTools tree. It has a universal concept of "children" (that's why it's a tree). Composites "contain" what they render, and hosts contain what's inside of them. <div><Foo /></div> is displayed in its fully expanded form: <div><Foo><Bar><h1>I am bar content</h1></Bar></Foo></div>.

I think for the same reason distinction between rendered and children is unnecessary. It is always up to traversing code "where to stop" unwrapping the tree. Composite component's children are what they render, and host component's children are based directly on what was passed to them. And you can always check if something is a composite or not by its type.

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. You're right that this proposal has a "bug" in it when I commented that rendered is null for host nodes. I think that's incorrect.

I see what you are saying w.r.t. a discrepency here with children and rendered.

I think it would be good for me to write out some example components and what the corresponding RST would be so that we can discuss in more concrete terms.

In your example:

<div>
  <Foo>
    <Bar>
      <h1>I am bar content</h1>
    </Bar>
  </Foo>
</div>

Since this is a true (ie, one concept of children), we've lost the concept of ownership here, so we aren't able to identify that <Foo /> was rendered with no children, unless we keep track of the "prop" children separate from the concept of true children:

What I'm trying to accomplish is a data structure that allows us to implement shallow rendering + full rendering and anywhere in-between. Let me try to add some examples to this document, as I'm reaching the limit of what I can reason about in my head :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there was some markup that disappeared in your comment.
What I meant to say is you can always grab original children from the props. So that should satisfy:

we aren't able to identify that was rendered with no children

Choose a reason for hiding this comment

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

The way i see rendered being different is that given when type is a function(Component constructor) rendered will represent the node the component returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand but the distinction is already derivable from the type in cases where it's important (and I'm not sure how important it is in practice when writing APIs like find()).

I'd aim for avoiding duplicate/derivable information in the tree representation.

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'd aim for avoiding duplicate/derivable information in the tree representation.

Agreed!

The reason I still feel like we need a standalone children property is because I want to be able to use this representation to recreate the current behavior that enzyme's shallow API has. This uses the shallow renderer, which enzyme basically uses to traverse the result of render of a single component.

The result of render is a tree of ReactElements. With this proposal in place, enzyme wouldn't know how to traverse a tree of ReactElements, but it would know how to traverse a tree of Nodes.

I'm not sure if my comments were accurate or not, and I'm going to give this more thought, but I'm pretty sure children is necessary if we aim to reimplement shallow on top of this spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does my comment in #742 (comment) help? I see shallow rendering as just a special case of normal test rendering, but with "restrictive" mocking. And test renderer would spit out RST nodes anyway.

// This can feel similar to children, but it's not the same thing at all. For a given node,
// this corresponds to the result of the `render` function with the provided props, but transformed
// into an RST. For "host" nodes, this will always be `null`.
rendered: Node?
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 (and Preact/Inferno?) a composite may return a fragment (multiple elements).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. I think in this case we would define Node as a recursive type:

type Node = Literal | RSTNode | [Node]

Would that work?

Copy link

@thysultan thysultan Dec 20, 2016

Choose a reason for hiding this comment

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

the nodeType flag could handle such cases since fragments could just be another type of Node i.e

{
nodeType: 11,
type: 'fragment',

...

children: [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.

true. we could get rid of the idea of Node vs RSTNode and instead just represent an RSTNode as a disjoint union of types, hinging on nodeType

At the moment I don't have any context into what coroutines and portals would mean for this proposal... might have to do some reading into that...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful to represent fragments as nodes? Or should their output be "inlined" into the thing that renders a fragment? I'm not sure what's more useful in your use case.

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 because they would have independent backing instances, we would want them to be separate nodes

Copy link
Contributor

Choose a reason for hiding this comment

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

Fragments wouldn't have backing instances. They are "invisible" to the application developer. Just like <div>{[[[<div />], <span />], <p />]}</div> would be "the same" as <div><div /><span /><p /></div> to anything outside the component itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so if I have a component Foo that renders [<div />, <span />], is the right mental model to change that in my head to something like <react:fragment>{[<div />, <span />]}</react:fragment>?

If so, I wasn't understanding earlier, as I was thinking it would have been the node with type: Foo that would have nodeType: ReactFragment.

Copy link
Contributor

Choose a reason for hiding this comment

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

is the right mental model to change that in my head to something like <react:fragment>{[<div />, <span />]}</react:fragment>?

This is the right model.

// 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it matters for your use cases but React "instances" can also have a key associated with them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I was considering adding key here, but i think maybe it isn't actually needed for enzyme's purposes. Perhaps it is for others?

// 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's useful for your use case but we also keep track of "source location" for components in development. We read it from the element.__source property when this transform is enabled. We use it for element stacks in warnings but I imagine you could use this for friendlier errors in assertions maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Will this just show up as a prop though? In that case we wouldn't need to make it part of this proposal, but enzyme could indeed look for this prop to give better errors if it exists

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's right on the element object itself.

type RendererOptions = {
// I'm not 100% sure about this, but I think with an API like this, we could implement
// `mount` and `shallow` with the same renderer.
?isLeaf(Element): bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, not sure this works, since you have no access to depth, so how would you implement the current shallow renderer, { isLeaf: () => true }? does that mean this function would be called for all elements other than the one you pass to EnzymeRenderer.render?

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 should cook up an example for this as well. As i'm thinking about this more, I don't think this really enables us to emulate shallow exactly, but is definitely some functionality i've been wanting for configurable depth rendering. I'll have to think about this a little bit more. If you have any ideas def. let me know.

Copy link
Contributor

@gaearon gaearon Dec 21, 2016

Choose a reason for hiding this comment

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

It seems like you want resolveType(type) => type that is identity by default. Then shallow rendering of Foo is resolveType = type => type === Foo ? ShallowWrapper : type. And ShallowWrapper would render null by default but its node would still have the original type so it would "appear" to be Foo in traversal. But you could also use resolveType(type) more generically, e.g. resolveType = type => mockedComponents[type] || type.

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 Yeah, I agree. Shallow rendering is effectively the same as mocking.

Would it render null or this.props.children though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imagination fails me, need to see a proof of concept first. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might be on point with props.children.

Copy link
Collaborator

@nfcampos nfcampos Dec 21, 2016

Choose a reason for hiding this comment

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

I've always imagined shallow rendering as effectively treating composite components as host components, so it would make sense for it to "render props.children" I think

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.

type ComponentInstance = {
state: object?
context: object?
setState: function
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing refs on purpose?

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 don't think we should standardize on refs. As it stands right now, it's a behavior that's being "deprecated" in favor of people handling refs with functions themselves

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fair. But what impact will that have on the Enzyme API. Do we have to deprecate the .ref api if we do not have a safe way to look it up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide an API to read arbitrary fields from the instance? That's what callback refs set.

Copy link
Member

Choose a reason for hiding this comment

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

How would we be able to know, from a callback ref, what changes were made? Sometimes people store the refs in an object, and sometimes they're not stored at all :-/

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 leaves the world in no different of a place than where we already are. As it stands, the ref API on enzyme only works with people using the string ref API, and using instance().nameOfProperty works and always has. This proposal needs only be about traversing the tree. Everything else can and will work identically to how it has.

@lelandrichardson
Copy link
Collaborator Author

lelandrichardson commented Jan 24, 2017

I've revised this proposal, considering some of the feedback given here. I've chatted offline with @ljharb about this at length, and think that this is generally moving in the right direction.

Some thoughts around the changes:

1. Do we need a nodeType property?

This is still debatable. I don't think we need to care about "fragment" types in the way
the spec is now, but we could alter the proposal to wrap literal values and other things into
nodes of the graph with a specified nodeType, and represent an RSTNode as a disjoint union
type. I feel like the current approach is pretty reasonable though.


2. What about state, context, refs, etc.?



These APIs are not needed for traversal and are not going to be included in this spec. This spec
is explicitly stating that the instance property on an RSTNode implements the public API of a 
React Component in the corresponding release signified by getTargetVersion. As a result, we are
effectively saying that other than the specified structure of the RSTNode, Enzyme will be
following whatever other API React chooses to go with.




3. Do we need both rendered and children?

Ultimately I decided that Dan was right about this. I do think there is more information provided
by rendered and children combined, but I don't actually think that it's useful to us, and ends
up creating a LOT of duplicate data in the tree. I've decided to use rendered as the namespace
because this is semantically more accurate, and not as ambiguous. With the setup I've laid out here,
"Host" nodes will end up having rendered be an Array, whereas "Composites" will end up having
rendered be another RSTNode. When you think about it, this makes a bit of sense: "Composites"
are in effect "wrapping" Host nodes.




4. What about Portals and Coroutines?



I still don't know what these mean for this proposal. If anyone has thoughts on this, please let me 
know.




5. Can we achieve shallow with just isHost(Element): boolean?



At the moment, I think the answer to this is "sort of, but with a caveat". I think
we certainly can if we say that the isHost function is not called on the root element that gets
passed into the renderer. In this case, isHost: () => true would be all we would need to implement
the shallow renderer. This actually seems pretty reasonable to me as a solution.

6. For "host" nodes, can instance ever be non-null?


I haven't specified this yet. It doesn't have a ComponentInstance, but we could have it return 
the actual DOM node if we are working with something like mount. Could it be the DOM element? Yes/No??



Let me know what ya'll think. If we can get alignment on this, I think we can start working on implementation in this branch. My hope is that we can get something like this done before React 16 so we can be sure there are no big barriers for people migrating.


## 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

## Motivation

This proposal is attempting to address a hand full of pain points that Enzyme has been
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 ;-) )

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.

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?

@blainekasten
Copy link
Contributor

  1. What about state, context, refs, etc.?
    ...Enzyme will be
following whatever other API React chooses to go with.




I think it's worth documenting this API regardless so adapter implementors have to only look at the spec to build an adapter.

@lelandrichardson
Copy link
Collaborator Author

@blainekasten I hesitate to document it because I don't want to make this spec more constraining for the react core team than it has to be. It is their responsibility to decide what the public API of react is, and I don't want enzyme to be sending different messages of them. I see this as our way of saying enzyme is following the changes of react, not the other way around (and thus adapter maintainers would be expected to do the same).

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.

@danielbayerlein
Copy link

Awesome! I'm very excited to use it! 💃

@IgnusG
Copy link

IgnusG commented Jun 22, 2017

Is this proposal still valid or has it been postponed/scraped?

@ljharb
Copy link
Member

ljharb commented Jun 22, 2017

@IgnusG still valid.

@ljharb
Copy link
Member

ljharb commented Aug 15, 2017

Since #1007 is merged, I think this can be closed. Feel free to reopen if not.

@ljharb ljharb closed this Aug 15, 2017
@ljharb ljharb deleted the enzyme-harmony branch August 15, 2017 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.