-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Refactor enzyme to use Adapters, initial React 16 support #1007
Conversation
5e069dc
to
c98e97c
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nbd but you can use 1.
for all of these, and then reordering won't require a diff on the entire list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. i usually avoid this because when reading it in plain text it gets confusing
``` | ||
|
||
```js | ||
expect(wrapper.props()).to.deep.equal({ outer: x }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this example show the same thing with shallow
, to underscore the parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. this document isn't done yet
docs/future/migration.md
Outdated
|
||
## Refs | ||
|
||
Refs no longer return a "wrapper". They return what the ref would actually be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should elaborate on what the options are (iirc, null/undefined or a DOM node?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or a composite component instance?
docs/future/migration.md
Outdated
|
||
## Keys | ||
|
||
keys no longer work? we should maybe fix this in the spec... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a TODO for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there already is one in the PR description
docs/future/migration.md
Outdated
|
||
## for shallow, getNode() was renamed to getElement() | ||
|
||
## for mount, getNode() should not be used. instance() does what it used to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused - getNode()
should have been returning a DOM node, and instance()
should return an instance of a component. Has this changed for mount
?
If we're renaming getNode
on shallow, let's do the same for mount - such that neither has getNode
or .node
or .nodes
anymore (so people's tests will fail and they can do any needed migration)
src/adapters/ReactSixteenAdapter.js
Outdated
import React from 'react'; | ||
import ReactDOM from 'react-dom'; | ||
import ReactDOMServer from 'react-dom/server'; | ||
// import TestRenderer from 'react-test-renderer'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make sure to do a sweep and remove all unneeded commented-out code before merging :-)
); | ||
} | ||
|
||
class EnzymeAdapter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see anywhere we're checking instanceof EnzymeAdapter
- it seems like we'd want to enforce people use this interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the strategy for exposing this class for adapter authors? Will it just be accessible from a lib
folder like
import EnzymeAdapter from 'enzyme/lib/EnzymeAdapter'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use it and enforce it, it'd need to be its own package so things could depend on it, and semver would help identify incompatibilities sooner.
src/ShallowWrapper.js
Outdated
@@ -1078,13 +1100,14 @@ class ShallowWrapper { | |||
dive(options = {}) { | |||
const name = 'dive'; | |||
return this.single(name, (n) => { | |||
if (isDOMComponentElement(n)) { | |||
if (n && n.nodeType === 'host') { | |||
throw new TypeError(`ShallowWrapper::${name}() can not be called on DOM components`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this phrase - "DOM components" - come from the adapter, and not from enzyme itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just going to change the terminology to "Host Components". This PR moves enzyme to an adapter architecture, but removing concepts of react entirely from enzyme and making it possible to use enzyme for react-like libraries is close, but will require some additional refactoring i'm sure. For things like terminology of "DOM Components" etc, I think these ideas are fairly baked in in other parts of the code as well. I think that that's okay at this point.
src/ShallowWrapper.js
Outdated
@@ -684,6 +699,11 @@ class ShallowWrapper { | |||
'a context option', | |||
); | |||
} | |||
if (this.instance() === null) { | |||
throw new Error( | |||
'ShallowWrapper::context() can only be called on class components as of React 16', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like an error that should come from the react 16 adapter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(TODO)
src/ReactWrapper.jsx
Outdated
this.component.setChildProps(props, callback); | ||
this.component.setChildProps(props, () => { | ||
this.update(); | ||
callback(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably check that it's a function here, just like on line 309
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we should check that it's a function (rather than letting an error be thrown async), should this still default to null or undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it should default to a noop because the previous behavior was that it wasn't required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right - it's not required here tho either. defaulting it to undefined means we'd throw if a non-undefined non-function was provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm adding a typeof check before the setChildProps
call. But keeping it defaulting to noop
. Seem reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I guess - it just adds function call overhead in the case when nothing is provided, so i'm not sure why this is better than defaulting to undefined
.
## Motivation | ||
|
||
This proposal is attempting to address a handful of pain points that Enzyme has been | ||
subject to for quite a while. This proposal has resulted mostly [#715](https://github.com/airbnb/enzyme/issues/715), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the second sentence seems to be missing some word somewhere, maybe "from" after "mostly"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass, I haven't looked through all files yet
docs/future/compatibility.md
Outdated
|
||
```js | ||
// Strings and Numbers are rendered as literals. | ||
type LiteralValue = string | number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If numbers are coerced to strings early on, will a LiteralValue ever be a number, or only a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is react 16 specific behavior that i don't think we should put in the spec...
// A "node" in an RST is either a LiteralValue, or an RSTNode | ||
type Node = LiteralValue | RSTNode | ||
|
||
// if node.type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean?
docs/future/compatibility.md
Outdated
|
||
// 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with react 16 can't this also be an array of RSTNode?
docs/future/compatibility.md
Outdated
// 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`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this bit sounds confusing, it makes me consider that this function should be renamed to something like shouldConsiderCompositeAsHost
?
} | ||
|
||
type EnzymeAdapter = { | ||
// This is a method that will return a semver version string for the _react_ version that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_react_ -> `react`
docs/future/migration.md
Outdated
|
||
## Refs | ||
|
||
Refs no longer return a "wrapper". They return what the ref would actually be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or a composite component instance?
docs/future/migration.md
Outdated
wrapper.find('.async-btn').simulate('click'); | ||
setImmediate(() => { | ||
// this didn't used to be needed | ||
wrapper.update(); // TODO(lmr): this is a breaking change... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a pretty big breaking change that we should try very hard to avoid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoping this doesn't fall through the cracks -- is there something about the new architecture that prevents an adapter from always grabbing from the latest output, or node, whenever a traversal starts? (i.e. myRootWrapper.find(...)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the new architecture makes this fundamentally not possible. I don't see any way around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to get this working as a POC: jwbay@df609cf
Since the wrapper .getNode functions are already the starting access point for all traversals, we can just get the latest root node there.
The only downside is we lose reference equality for nodes during traversal in some cases now that there's always a new object literal created in the adapter's getNode. I tried caching based on render output but it didn't make much of a difference because most traversals end up going through .wrap, which creates a new renderer.
It's probably also worth noting that mount
now also requires .update
in places it didn't before. This new test fails in this branch but passes in master: jwbay@c14e87b
docs/future/migration.md
Outdated
|
||
|
||
|
||
## Enzyme.use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this? Enzyme.configure?
docs/future/migration.md
Outdated
- x make sure all react dependence is moved into adapters | ||
- x make tests run for all adapters | ||
- export tests for 3rd party adapters to run | ||
- check the targetApiVersion returned by the adapter and use the semver library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use it for what? maintain a map of react api versions to available api methods? and warn/throw accordingly if an unavailable method is used?
package.json
Outdated
@@ -21,13 +21,14 @@ | |||
"test:watch": "mocha --recursive --watch test", | |||
"test:karma": "karma start", | |||
"test:env": "sh ./example-test.sh", | |||
"test:all": "npm run react:13 && npm run test:only && npm run react:14 && npm run test:only && npm run react:15.4 && npm run test:only && npm run react:15 && npm run test:only", | |||
"test:all": "npm run react:13 && npm run test:only && npm run react:14 && npm run test:only && npm run react:15.4 && npm run test:only && npm run react:15 && npm run test:only && npm run react:16 && npm run test:only", | |||
"clean-local-npm": "rimraf node_modules/.bin/npm node_modules/.bin/npm.cmd", | |||
"react:clean": "npm run clean-local-npm && rimraf node_modules/react node_modules/react-dom node_modules/react-addons-test-utils node_modules/react-test-renderer && npm prune", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add create-react-class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't create-react-class not need to be removed because it'd jut be ignored in older versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that’s true yes, but I’d rather test on environments closer to what an actual person eg. using react 13 would have (ie. no create react class installed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(TODO)
setupAdapters.js
Outdated
let Adapter = null; | ||
|
||
if (Version.REACT013) { | ||
Adapter = require('./src/adapters/ReactThirteenAdapter'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if enzyme went with the Lerna approach and published individual packages for each version? So if I only wanted react15, I would install the enzyme
and enzyme-adapter-react15
packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that the plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps? Doesn't look to be from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not part of this PR, but it's absolutely the plan. See "Followup PR moving the adapters into their own packages" in the OP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, missed that sentence.
docs/future/compatibility.md
Outdated
lineNumber: number | ||
|} | ||
|
||
type NodeType = 'class' | 'function' | 'host'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current toTree
implementation in react-test-renderer
only uses "component"
and "host"
for nodeType
.
docs/future/compatibility.md
Outdated
import Enzyme from 'enzyme'; | ||
import ThirdPartyEnzymeAdapter from 'third-party-enzyme-adapter'; | ||
|
||
Enzyme.configure({ adapter: ThirdPartyEnzymeAdapter }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.configure
would make more sense if there were long-term plans to provide other top-level configuration options via Enzyme.configure
. Otherwise 👍 for .use
docs/future/compatibility.md
Outdated
|
||
### Validation | ||
|
||
Enzyme will provide an `validate(node): Error?` method that will traverse down a provided `RSTNode` and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What node would Enzyme call it with at Enzyme.configure
?
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just required that adapters be explicitly configured, would we need this getTargetApiVersion
API at all? What happens if a React-like adapter (Preact, Inferno) exists for a version that collides with a React version? e.g., Preact has a 15.5 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. i think you're right. I think I will get rid of this, and let node semver declarations take care of the rest.
src/ShallowWrapper.js
Outdated
// React <15.5: Fallback to ReactDOM | ||
return batchedUpdates(fn); | ||
const adapter = configuration.get().adapter; | ||
// TODO(lmr): warn about no adapter being configured |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this throw instead of warn?
) { | ||
instance.componentDidMount(); | ||
} | ||
this.renderer = getAdapter(options).createRenderer({ mode: 'shallow', ...options }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If mode
is being used by Enzyme internals should it be added to the RendererOptions
type definition?
} | ||
} | ||
|
||
function propsOfNode(node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSTTraversal
exports a propsOfNode
utility, should we unify these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps, yes. I've been trying to separate things mentally between adapters and the core enzyme lib so that they are easy to separate, and it's easier for me to find when things are no longer used
c98e97c
to
538550f
Compare
What do you all think about adding pointers for |
@aweary doesn't that open up the possibility that |
@ljharb we already trust that the adapter is returning an accurate |
@lelandrichardson I tried this out on my tests and I ran into a couple of issues:
The following test passes against master: it('should simulate events on child nodes', ()=>{
class Foo extends React.Component {
constructor(props) {
super(props);
this.state = { count: 0 };
this.incrementCount = this.incrementCount.bind(this);
}
incrementCount() {
this.setState({ count: this.state.count + 1 });
}
render() {
return (
<div>
<a
className={`clicks-${this.state.count}`}
onClick={this.incrementCount}
>
foo
</a>
</div>
);
}
}
const wrapper = mount(<Foo />);
expect(wrapper.find('.clicks-0').length).to.equal(1);
wrapper.find('a').simulate('click');
expect(wrapper.find('.clicks-1').length).to.equal(1);
}); |
@TiraO "working out of the box by default" is the current state, and it's why we have issues with dependencies. It's important that users explicitly choose the version of React (or "not React") they're using. |
@ljharb Fair enough. Would there be both a react 16 native adapter and a react 16 dom adapter? |
@TiraO it would be possible to create one for each; i'm not sure what we're planning on releasing with, but the whole concept is that it's extensible. |
Is What about merging this PR into a branch like |
a315d72
to
04c4a67
Compare
with "React-like" libraries such as Preact and Inferno. | ||
|
||
We have done our best to make Enzyme v3 as API compatible with v2.x as possible, however there are | ||
a hand full of breaking changes that we decided we needed to make, intentionally, in order to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/hand full/handful
on React 16, you would want to configure Enzyme this way: | ||
|
||
```js | ||
import Enzyme from 'enzyme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this example use import { configure } from 'enzyme'
, even though both ways will work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aesthetically i kinda prefer this way for some reason, but i don't think i can justify it in any way :P
``` | ||
|
||
In this code, there is a timer that continuously changes the rendered output of this component. This | ||
might be a reasonable thing to do in your application. The thing is, Enzyme has no way of knowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to lowercase enzyme
throughout?
@@ -110,7 +105,7 @@ export function buildPredicate(selector) { | |||
return node => nodeHasId(node, selector.slice(1)); | |||
|
|||
case SELECTOR.PROP_TYPE: { | |||
const propKey = selector.split(/\[([a-zA-Z-]*?)(=|])/)[1]; | |||
const propKey = selector.split(/\[([a-zA-Z][a-zA-Z_\d\-:]*?)(=|])/)[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tests are passing just fine on master :-/ if you revert this change, does everything still pass?
|
||
eventFn(findDOMNode(n), mock); | ||
this.renderer.simulateEvent(n, event, mock); | ||
this.root.update(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this guaranteed to happen after the simulated event is finished?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing really is guaranteed other than simulatedEvent()
finishing here... which will call the event I believe (at least our tests indicate), but of course the event could schedule something async
@@ -600,7 +657,7 @@ class ReactWrapper { | |||
* @returns {ReactWrapper} | |||
*/ | |||
children(selector) { | |||
const allChildren = this.flatMap(n => childrenOfInst(n.getNode())); | |||
const allChildren = this.flatMap(n => childrenOfNode(n.getNodeInternal()).filter(x => typeof x === 'object')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be nice to have a named isObject
function, or, to have childrenOfNode
take an arg, or have a variant, that only returns non-primitives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge - we've discussed some post-PR pre-v3 additional tasks, including removing some internal properties; restructuring the adapters into a multi-package-repo structure; adding .root()
and making mount's and shallow's methods consistent about what the wrapper "is" conceptually; and addressing any extant TODOs and PR comments.
Thanks for all this work, v3 is going to be great!
Any sense when a release might be made available with this change? |
None yet. |
any update on release with these changes? |
to: @ljharb @aweary @nfcampos @blainekasten
cc: @sebmarkbage @developit @bvaughn
This PR is effectively a rewrite of Enzyme's internal machinery, per the discussion outlined in #742
I attempted to make this as small of a breaking change as pragmatically possible, and minimize the amount our current unit tests needed to be changed across the refactor.
Motivation
This PR attempts to address a handful of pain points that Enzyme has been
subject to for quite a while.
The desired results are the following:
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
andshallow
.Additional libraries can provide compatible adapters
React API-compatible libraries such as
preact
andinferno
will be able to provide adapters to Enzymefor their corresponding libraries, and be able to take full advantage of Enzyme's APIs.
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 is minimized to the adapter implementations only, which will be imported separately, so bundlers will not have any of these problems.
Standardization and interopability with other tools
With an agreed on tree format, 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.
Things left to do before merging this PR:
Things left to do before publishing Enzyme 3.0: