Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support React.lazy and React.Suspense #1975

Merged
merged 3 commits into from Apr 24, 2019
Merged

Conversation

chenesan
Copy link
Contributor

@chenesan chenesan commented Jan 10, 2019

Fixes #1917 .

Given a component wrapped by React.lazy in <Suspense />
It'll plainly render a <Lazy /> in shallow and render fallback component in mount.

There are something I'm not sure / still working on:

  1. What should displayName of the component returned by React.lazy be? Currently I directly named it as Lazy. Not sure if it's something we could define by ourselves.

  2. I'm trying to add an waitUntilLazyLoaded() on ReactWrapper, which will return a promise resolving when the dynamic import loaded and React trigger the re-render, so we can write some tests like:

const LazyComponent = lazy(() => import('/path/to/dynamic/component'));
const Fallback = () => <div />;
const SuspenseComponent = () => (
    <Suspense fallback={<Fallback />}>
      <LazyComponent />
    </Suspense>
);

const wrapper = mount(<SuspenseComponent />)
await wrapper.waitUntilLazyLoaded()

expect(wrapper.find('DynamicComponent').to.have.lengthOf(1)

But I don't know how to detect if all the lazy loaded component inside <Suspense /> has completeted loading. It looks like that we have to hack around react fiber. @ljharb Would you know any way to detect this?

Also note that this PR add babel-plugin-dynamic-import-node and babel-plugin-syntax-dynamic-import for using import(), babel-eslint in enzyme-adapter-react-16 and enzyme-test-suite for dynamic import support of eslint.

@rodoabad
Copy link

@chenesan are you not able to determine the display name of the component that is being loaded by React.lazy? Once the promise has resolved you should have full access to the lazy loaded component correct?

@chenesan
Copy link
Contributor Author

@chenesan
Copy link
Contributor Author

@ljharb I just force pushed the branch and I think it's ok to review it now.

Basically this PR will supports shallow/mount with Suspense and React.lazy.

@chenesan chenesan changed the title [WIP] Support React.lazy and React.Suspense Support React.lazy and React.Suspense Jan 15, 2019
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great start.

.babelrc Outdated
@@ -2,6 +2,8 @@
"presets": ["airbnb"],
"plugins": [
["transform-replace-object-assign", { "moduleSpecifier": "object.assign" }],
"syntax-dynamic-import",
"dynamic-import-node"
Copy link
Member

Choose a reason for hiding this comment

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

let's only add these in the "test" env; i don't think this is ever a safe transform except at the app level.

@@ -1,5 +1,6 @@
{
"extends": "airbnb",
"parser": "babel-eslint",
Copy link
Member

Choose a reason for hiding this comment

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

this is also not acceptable; babel-eslint enables all syntax currently, so airbnb projects only use the default parser.

Suggested change
"parser": "babel-eslint",

@@ -38,6 +67,9 @@ module.exports = function detectFiberTags() {
// eslint-disable-next-line no-unused-vars
FwdRef = React.forwardRef((props, ref) => null);
}
if (supportsLazy) {
LazyComponent = React.lazy(() => import('./_helpers/dynamicImportedComponent'));
Copy link
Member

Choose a reason for hiding this comment

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

There's zero point in using import() here - it has to be transpiled, and it's never safe to transpile it below the app level, so this would have to go untranspiled, and that'd be a breaking change.

It'd be much simpler to use () => Promise.resolve().then(() => ({ default: require(…) })).

Separately, I'm not sure why this can't be even simpler - React.lazy(() => Promise.resolve().then(() => ({ default() { return null; } }))

Copy link
Contributor Author

@chenesan chenesan Jan 16, 2019

Choose a reason for hiding this comment

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

Ah... yeah, the Promise.resolve().then(() => ({ default() { return null; } })) indeed works. If so I think all the added eslint plugins for import syntax, fake module file and babel-eslint can be removed. Thanks for this solution!

Copy link
Member

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, this way is much cleaner in the sense that it preserves a single source of truth of "how import() should work in node" - but due to the underspecified nature of ESM, until node ships unflagged ESM, it's not safe for anything but the top-level app to define "how import() works", unfortunately.

Copy link
Contributor Author

@chenesan chenesan Jan 16, 2019

Choose a reason for hiding this comment

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

I think maybe it's worth to add a utility to make it clearer like

const fakeDynamicImport = (module) => Promise.resolve().then(() => ({ default: module}))

// so we can write
const LazyComponent = React.lazy(() => fakeDynamicImport(SomeComponent))

for all the places using React.lazy in enzyme packages. But I'm not sure where to put this utility.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great! If it'd be used by only adapters, it should be in adapter-utils; if by enzyme as well, then probably in enzyme itself; since it's likely to only be used in the 16 adapter for now but the 17 adapter later, probably in adapter-utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

React.lazy only appears in 16 adapter and tests. Looks like adapter-utils is a good place.

@@ -1,6 +1,7 @@
{
"extends": "airbnb",
"root": true,
"parser": "babel-eslint",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"parser": "babel-eslint",

"object-inspect": "^1.6.0",
"object.assign": "^4.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

not sure why this got re-sorted, what version of npm did you use to install it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like I accidentally add new package with yarn. will fix this.

@@ -0,0 +1,5 @@
import React from 'react';

const DynamicComponent = () => <div>Dynamic Component</div>;
Copy link
Member

Choose a reason for hiding this comment

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

this entire file seems unnecessary if it exists in enzyme


const wrapper = shallow(<SuspenseComponent />);

expect(wrapper.find('Suspense')).to.have.lengthOf(1);
Copy link
Member

Choose a reason for hiding this comment

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

Since a shallow wrapper is "what the wrapped thing renders", i would expect this to be:

Suggested change
expect(wrapper.find('Suspense')).to.have.lengthOf(1);
expect(wrapper.is(Suspense)).to.equal(true);


expect(wrapper.find('Suspense')).to.have.lengthOf(1);
expect(wrapper.find(LazyComponent)).to.have.lengthOf(1);
// won't render fallback in shallow renderer
Copy link
Member

Choose a reason for hiding this comment

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

can we add another test that .dive()s the Suspense, and then becomes the fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I forgot to test shallow render Suspense directly, and I found out that for now react-test-renderer/shallow doesn't allow render Suspese:

import ShallowRenderer from "react-test-renderer/shallow"

const renderer = new ShallowRenderer()
renderer.render(<Suspense fallback={false} />)

will throw:

Invariant Violation: ReactShallowRenderer render(): Shallow rendering works only with custom components, but the provided element type was `symbol`.
    at invariant (/Users/yishan/Documents/Projects/enzyme/packages/enzyme-adapter-react-16/node_modules/react-test-renderer/cjs/react-test-renderer-shallow.development.js:54:15)
    at ReactShallowRenderer.render (/Users/yishan/Documents/Projects/enzyme/packages/enzyme-adapter-react-16/node_modules/react-test-renderer/cjs/react-test-renderer-shallow.development.js:382:78)

It looks like that react shallow renderer cannot recoginze the Suspense type symbol. I should check this at first... So unless react-test-renderer supports this I think we can only support Suspense and lazy in mount. 😢


const wrapper = shallow(<SuspenseComponent />);

expect(wrapper.find('Suspense')).to.have.lengthOf(1);
Copy link
Member

Choose a reason for hiding this comment

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

Since a shallow wrapper is "what the wrapped thing renders", i would expect this to be:

Suggested change
expect(wrapper.find('Suspense')).to.have.lengthOf(1);
expect(wrapper.is(Suspense)).to.equal(true);

}
}
const SuspenseComponent = () => (
<Suspense fallback={false}>
Copy link
Member

Choose a reason for hiding this comment

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

also here, let's pass a Fallback

@chenesan
Copy link
Contributor Author

Fixed problems except the .dive() one, which makes me find out that react-test-renderer/shallow not support render Suspense. Feel sorry about not checking this earlier :(

@chenesan
Copy link
Contributor Author

@ljharb I'm wondering if I should work on shallow renderer in react side to make progress for this PR.
If so, I'm not sure how shallow renderer should handle fallback of Suspense. Should we just return the children, or traverse the children of Suspense to find all lazy component and transform this to fallback element? I know this seems a bit off topic here, though.

@chenesan
Copy link
Contributor Author

Hi @ljharb I tried to start a PR in facebook/react#14638 to support shallow rendering Suspense in react shallow renderer. I'm not sure if I did it right for enzyme. I guess you may be interested in looking into this ;)

@chenesan
Copy link
Contributor Author

I'm not sure when react-test-renderer/shallow will support Suspense and it looks like it'd need a long time(has started a PR in facebook/react#14638 and waiting for reply). So I think we can just concentrate on supporting Suspense and lazy in mount in this PR to make it work sooner. Once shallow renderer support this we can create a new PR. I just removed the shallow wrapper test to prevent misleading.

@ljharb I think we can start a new round of review. If you think it's better to wait for shallow renderer support, let me know and I'll add tests back, thanks :)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This looks good, but I think we should re-add the shallow tests.

If the shallow renderer won't support it, then we'll have to support it for them.

@chenesan
Copy link
Contributor Author

ok, I'll add them back. If there's still no reply in react this week I'll start to work on support Suspense in adapter 16.

And if we have to support Suspense in enzyme side, how should we handle the fallback inside Suspense? I think we should keep behavior of shallow as same as mount, and we could just turn unresolved lazy component node in children to fallback node. But if we keep .dive() down into the children and get a lazy component node, should we handle fallback for it? Or we should just throw error? I'd prefer to the latter since React doesn't support render lazy component independently, but I'm not sure if this behavior would confuse user.

@ljharb
Copy link
Member

ljharb commented Jan 22, 2019

@chenesan it kind of depends on what react's planning to do.

In other words, in general, we want shallow and mount to be as close as possible. However, when you're shallow rendering something that renders a Suspense or a lazy, those things should just show up in the tree - when you dive through one of them, or when you shallow render one of them directly, then their implementation should be "invoked", whatever that ends up meaning.

Similarly, it should be possible (in both shallow and mount) to force a Suspense to render the lazy component or the fallback, so both paths can be tested.

@chenesan
Copy link
Contributor Author

So when we shallow render Suspense

const LazyComponent = React.lazy(() => import('InternalComponent'))
shallow(<Suspense fallback={<Fallback />}>
  <LazyComponent />
</Suspense>)

then should it render a fallback, or the <LazyComponent /> -- but not invoke the lazy load function (which should be invoked when we shallow render <LazyComponent />?

And another problem is when we shallow(<LazyComponent />), there seems to be some possible behaviors:

  1. throw error, since React not allow singly render lazy component
  2. wait it load until it resolved or error, if error we have to give it a fallback. But it's tricky to wait for loading in tests.
  3. add option in shallow, or we could provide some utility resetLoader to replace LazyComponent loader function in run time, to replace LazyComponent with implementation when shallow rendering.

I think 1+3 sounds reasonable.

Similarly, it should be possible (in both shallow and mount) to force a Suspense to render the lazy component or the fallback, so both paths can be tested.

it sounds like we can pass an option when shallow rendering to determine how to render Suspense - maybe a "normal" mode to render them as same as what React does and a "fallback" mode to render them all as fallback.

@ljharb
Copy link
Member

ljharb commented Jan 23, 2019

then should it render a fallback, or the -- but not invoke the lazy load function (which should be invoked when we shallow render ?

Yes. Whether it should render the fallback or the LaxyComponent, I have no idea - (note that this only comes up when the Provider is the component being rendered).

As for LazyComponent, that's up to the author of the test - it seems like perhaps we only need a mechanism for the developer to instruct enzyme whether to use the fallback or to use the child.

@chenesan
Copy link
Contributor Author

ok, so I'll add an option (maybe suspenseFallback: boolean) into shallow and pass it down to adapter. When shallow(<Suspense fallback={<Fallback />}>/* children */</Suspense>), if it's true, it will traverse through the children and replace all of <LazyComponent /> with <Fallback />, else we will keep them there.
And for shallow(<LazyComponent />) just let shallow renderer throw error(maybe rethrow with an clearer error message).
I'll work on them after this week if there's still no feedback from react side. Thanks for discussion :)

@ljharb ljharb added this to v16.6: memo, lazy, suspense in React 16 Jan 23, 2019
@ljharb
Copy link
Member

ljharb commented Jan 23, 2019

Sounds great. The only concern I think that's left is "what happens when you shallow render a lazy component", and it'd be great to know what React plans in the shallow renderer before we implement it here (we don't have to wait on them releasing that, we just need to know what they'll do)

@chenesan
Copy link
Contributor Author

@ljharb Still not figured this out :-( Sometimes the coverage recovered after I removed some lines of code, but when I tried to reproduce again it still dropped. Wondering if it's an issue in istanbul side -- Do you have any idea around this?

@chenesan
Copy link
Contributor Author

@ljharb Get this done in e60788c. It seems in some places we import fakeDynamicImport from enzyme-adapter-utils/src/Utils rather than enzyme-adapter-utils, which cause nyc count coverage incorrectly.

@chenesan
Copy link
Contributor Author

@ljharb Sorry for pinging again :-| Just fixed the coverage issue in e60788c so I think this could be reviewed / merged now?

@ljharb ljharb merged commit b5afdb9 into enzymejs:master Apr 24, 2019
@DonikaV
Copy link

DonikaV commented May 2, 2019

Sorry, i have a question. When i can use enzyme, shallow with lazy and Suspense? Is there any updates of library in the future? Merge was 8 days ago. But i see that the version of enzyme is the same...
Thanks in advance.

@ljharb
Copy link
Member

ljharb commented May 2, 2019

This will be in the next release; there’s no timeline for it.

ljharb added a commit that referenced this pull request May 7, 2019
 - [new] add `fakeDynamicImport` (#1975)
 - [deps] update `airbnb-prop-types`
 - [dev deps] update `eslint-plugin-react`, `eslint-plugin-import`
ljharb added a commit that referenced this pull request May 12, 2019
 - [new] support `suspenseFallback` option; support `Suspense`/`Lazy` (#1975)
 - [fix] `shallow`: properly dive through `memo` components (#2103, #2068)
 - [fix] avoid a Context.Provider deprecation warning
 - [fix] shallow renderer for `memo` does not respect `defaultProps` (#2115)
 - [fix] Don’t show wrapped component displayName in lazy component (#1975)
 - [fix] `simulateEvent`: call the adapter’s implementation, not the raw one (#2100)
 - [deps] update `enzyme-adapter-utils`
 - [dev deps] update `eslint-plugin-react`, `eslint-plugin-import`
@GitHub-Mar
Copy link

I noticed this was released 3 days ago in v 1.13 of enzyme-adapter-react-16. Does Enzyme now fully support dynamically imported components and wrapping? We are still struggling to get this working for our local tests.

Is there any documentation or working examples someone could point me to? I've been trying to test a dynamically imported component that is not wrapped in a Suspense component as we are using a Suspense higher up the component tree to wrap everything, however, I'm unsure how to tell enzyme to resolve the promise and load through the component? If I debug I can see the node as a child of my mounted wrapper but it's just listed as a react.lazy.

I believe this is the correct strategy when using Suspense components, though I'm also quite new to this and may well be wrong!

@ljharb
Copy link
Member

ljharb commented May 14, 2019

@GitHub-Mar no, this pr has two components, enzyme itself (not released) and the adapter (released).

@GitHub-Mar
Copy link

Ah my mistake! So this feature is not currently supported in Enzyme, then? I'm guessing your above comment still stands and there is no timeline as to when this will be released?

@ljharb
Copy link
Member

ljharb commented May 15, 2019

Correct.

@ljharb
Copy link
Member

ljharb commented Jun 4, 2019

v3.10.0 has now been released.

@DonikaV
Copy link

DonikaV commented Jun 7, 2019

Still failing. Do you have some docs for this? I mean maybe i should update my tests or something like this?
Thanks

@ljharb
Copy link
Member

ljharb commented Jun 7, 2019

@DonikaV all the docs are published. If you've fully updated enzyme and your adapter, please file a new issue and fill out the template.

@shridharkalagi
Copy link

Can I use Enzyme mount API with Lazy and Suspense yet?

@ron23
Copy link

ron23 commented Dec 11, 2020

I'm still a bit confused on what's the best way to test that a component wrapped in lazy renders inside Suspense.
Say I have:

function myComponent = (props) => {
   const MyLazy = React.lazy(() => import('./MyLazyComponent');
   return <Suspense fallback={...}><MyLazy/></Suspense>
}

If I use the method mentioned above to mock react:

jest.mock('react', () => {
  const React = jest.requireActual('react');
  React.Suspense = ({ children }) => children
  return React;
});

The debug() returns something like:

<Component fallback={{...}}>
      <lazy />
    </Component>

I want to test that MyComponent actually renders MyLazyComponent.
I tried mount and shallow and neither works.

enzyme@3.11.0
enzyme-adapter-react-16@1.15.2

@linwei0201

This comment has been minimized.

@ljharb

This comment has been minimized.

@orlandovallejos

This comment has been minimized.

@ljharb

This comment has been minimized.

@ljharb

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
React 16
  
v16.6+: memo, lazy, suspense
Development

Successfully merging this pull request may close these issues.

Support Suspense