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

Open
wants to merge 2 commits into
base: master
from

Conversation

4 participants
@chenesan
Copy link
Contributor

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.

chenesan added some commits Jan 9, 2019

@rodoabad

This comment has been minimized.

Copy link

rodoabad commented Jan 13, 2019

@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

This comment has been minimized.

@chenesan chenesan force-pushed the chenesan:react-suspense branch from e656f7a to 3577a02 Jan 15, 2019

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 15, 2019

@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

@ljharb
Copy link
Member

ljharb left a comment

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"

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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",

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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

Suggested change Beta
"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'));

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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; } }))

This comment has been minimized.

@chenesan

chenesan Jan 16, 2019

Author Contributor

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!

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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.

This comment has been minimized.

@chenesan

chenesan Jan 16, 2019

Author Contributor

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.

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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?

This comment has been minimized.

@chenesan

chenesan Jan 16, 2019

Author Contributor

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",

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member
Suggested change Beta
"parser": "babel-eslint",
"object-inspect": "^1.6.0",
"object.assign": "^4.1.0",

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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

This comment has been minimized.

@chenesan

chenesan Jan 16, 2019

Author Contributor

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

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

this entire file seems unnecessary if it exists in enzyme


const wrapper = shallow(<SuspenseComponent />);

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

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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

Suggested change Beta
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

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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

This comment has been minimized.

@chenesan

chenesan Jan 16, 2019

Author Contributor

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);

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

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

Suggested change Beta
expect(wrapper.find('Suspense')).to.have.lengthOf(1);
expect(wrapper.is(Suspense)).to.equal(true);
}
}
const SuspenseComponent = () => (
<Suspense fallback={false}>

This comment has been minimized.

@ljharb

ljharb Jan 16, 2019

Member

also here, let's pass a Fallback

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 16, 2019

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 chenesan force-pushed the chenesan:react-suspense branch from 1652d74 to 1bbe447 Jan 17, 2019

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 17, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 19, 2019

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 chenesan force-pushed the chenesan:react-suspense branch from 1bbe447 to 7458330 Jan 21, 2019

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 21, 2019

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 :)

@ljharb
Copy link
Member

ljharb left a comment

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

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 22, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 22, 2019

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.

@chenesan chenesan force-pushed the chenesan:react-suspense branch from 7458330 to 8a1dde1 Jan 23, 2019

@ljharb

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 23, 2019

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

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 24, 2019

Thanks for reply :-) I agree that it's easier to directly test the wrapped component itself and just want to make sure how React will handle this. Will keep waiting for reply from gaearon.

Also note some people may shallow render<Suspense><LazyComponent /></Suspense>, and call .dive() to see what would be rendered inside lazy component. Not sure if we should support such use case. If React will not support shallow rendering lazy component then I think we could also not support this in enzyme.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jan 24, 2019

I don't feel comfortable saying what React will or will not do with regard to shallow rendering and lazy, but it does seem like a flawed premise. Really all that such a test case (<Suspense><LazyComponent /></Suspense>) would be testing is the renderer itself. What you'd really want to test is the component returned by LazyComponent. (I don't personally see much value in shallow testing that either, but at least you're "testing" your own component code at that point.)

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 24, 2019

@bvaughn the question is about testing a lazy component in the absence of suspense; the answer might very well be “pretend the lazy wrapper isn’t there”.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jan 24, 2019

Yeah, I understand the question. I'm questioning the premise.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 24, 2019

A module might export a lazy component, and the tests ideally shouldn’t have to know or care that it’s lazy - and if they do, they still need a way to test the unexported wrapped component, without a dom and without rendering the entire tree.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jan 24, 2019

lazy was designed to wrap an import, rather than to be the export. I think this pattern is more practical (since it defers the decision of code splitting to the caller) and sidesteps awkward testing issues like this (e.g. needing to wrap with <Suspense> and advance timers to process micro tasks, etc)

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 24, 2019

While that makes perfect sense as advice for the author of the code, as a testing framework, we can’t limit ourselves to what might be a better pattern - we have to have a reasonable answer for every edge case.

One might be “just pretend lazy isn’t there”. One might be “lazy, in shallow, is just a custom component that renders the thing it wraps”. One might be “it throws” (the least useful). Whatever is picked also determines how it will show up in debug, as well as the React dev tools.

Before enzyme implements anything, i want to know what the official react shallow renderer will do, and what the official react dev tools will do, and what react-is and propTypes will do - because one way or another, all these questions always need to be answered for every single feature React releases. Hopefully, such questions would be answered at least internally before releasing a feature - is there any official plan for these things regarding lazy?

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 25, 2019

hmm, (from my view) it looks like currently react is not interested in handling lazy in shallow renderer(or thinks it's a bad idea to shallow render lazy component for testing and users should directly shallow render the wrapped component). @gaearon (sorry for pinging you here again 😓 ) do you have any comments on @ljharb last comment? We might need a reply (even it's really no plan/interests on this) to go ahead (to determine what enzyme should do when people shallow(<LazyComponent />) for this PR, thanks :-)

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jan 25, 2019

While that makes perfect sense as advice for the author of the code, as a testing framework, we can’t limit ourselves to what might be a better pattern - we have to have a reasonable answer for every edge case.

We will have to disagree here then. I think that saying, "this is not a good pattern to use because of these reasons ___", is a reasonable answer.

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 25, 2019

"this is not a good pattern to use because of these reasons ___"

Does it mean "So to discourage this pattern, currently we have no plan to support this and will throw error when shallow rendering lazy component" ? Or it would be an undefined behavior?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 25, 2019

@bvaughn that's fine to have as a stance from React - but that has to translate to explicit, defined behavior for the shallow renderer and react dev tools, which enzyme will follow. So, what's the explicit behavior react is committing to here, even if it's "throwing"?

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jan 25, 2019

Does it mean "So to discourage this pattern, currently we have no plan to support this and will throw error when shallow rendering lazy component" ? Or it would be an undefined behavior?

I've already mentioned that I don't think I'm the best one to say what the React team will do with regard to this behavior in the future, because shallow rendering is not a feature I've paid much attention to. 😄 That being said, the shallow renderer currently only supports custom components (class or function) and fowrardRef. Other types (including React.Suspense) will tigger an error. I'm not aware of any plans in the near/mid future to change this.

Edit – Quick chat with the team confirms that we don’t plan on expanding support of the input arguments to shallow renderer.

@bvaughn that's fine to have as a stance from React - but that has to translate to explicit, defined behavior for the shallow renderer and react dev tools

Not sure I understand how React dev tools is related to this discussion. (They support lazy, as do ancillary packages like react-is.) The above statements are only scoped to testing with a shallow renderer.

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 26, 2019

Thanks for confirmation and response @bvaughn :-)
@ljharb if react has no plan for this, should we still try to handle lazy component in shallow? Personally I think we can throw for now because I'm not sure how many pepole would shallow rendering lazy component in their tests. If there are more feature requests and use cases then we could consider what to do next.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 26, 2019

It's fine if, for now, we throw on shallow rendering a lazy component, as long as we can ensure the same description of it in the debug tree as mount would have.

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 26, 2019

as long as we can ensure the same description of it in the debug tree as mount would have.

Sorry that I couldn't get what you mean 😓 . Did you mean .debug() output of mounting (including lazy component) should be as same as shallowing?

const element = <Suspense fallback={<Fallback />}>
  <LazyComponent />
</Suspense>
const mountWrapper = mount(element)
const shallowWrapper = shallow(element)

expect(mountWrapper.debug()).to.equal(shallowWrapper.debug())

in mount the <LazyComponent /> will be rendered as <Fallback /> in first render, and in shallow that will depends on the option(as above discussion) we passed into shallow() .

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 26, 2019

Let's move forward with this plan and with your understanding and we can tweak it as needed :-)

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Jan 28, 2019

@ljharb I added an option suspenseFallback into second arg of shallow. When it's true (default) it will replace all lazy component in children with fallback when rendering Suspense, or it will keep all lazy component there. Also throw error inside enzyme when shallow rendering lazy component.

@@ -559,7 +593,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
if (!inst || !this.isValidElement(inst)) {
return false;
}
return typeof inst.type === 'function' || isForwardRef(inst);
return typeof inst.type === 'function' || isForwardRef(inst) || isSuspense(inst);

This comment has been minimized.

@chenesan

chenesan Jan 28, 2019

Author Contributor

I think isCustomComponentElement should be renamed after adding Suspense, but cannot figure out a better name :(

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

this is basically isNonHostElement, but let's not worry about the name now

@chenesan chenesan force-pushed the chenesan:react-suspense branch from b6f5bdf to 61beb35 Jan 28, 2019

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Feb 5, 2019

@ljharb Is there anything I could do to progress this?

const adapter = this;
const renderer = new ShallowRenderer();
const { suspenseFallback } = options;
const shouldReplaceLazyWithFallback = typeof suspenseFallback === 'undefined' ? true : !!suspenseFallback;

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

let's throw here if typeof suspenseFallback isn't undefined or boolean

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

additionally, let's throw if this option is specified at all in the other two renderers.


let renderedEl = el;
if (isLazy(renderedEl)) {
throw Error('Enzyme doesn\'t support lazy component because React doesn\'t support it.');

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member
Suggested change Beta
throw Error('Enzyme doesn\'t support lazy component because React doesn\'t support it.');
throw TypeError('`React.lazy` is not supported by shallow rendering.');
@@ -344,9 +380,11 @@ class ReactSixteenAdapter extends EnzymeAdapter {
};
}

createShallowRenderer(/* options */) {
createShallowRenderer(options) {

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member
Suggested change Beta
createShallowRenderer(options) {
createShallowRenderer(options = {}) {

this keeps the .length the same.

@@ -537,6 +590,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
const name = type.render.displayName || functionName(type.render);
return name ? `ForwardRef(${name})` : 'ForwardRef';
}
case Lazy || NaN: return 'LazyComponent';

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

React.lazy doesn't seem to provide a display name - let's use:

Suggested change Beta
case Lazy || NaN: return 'LazyComponent';
case Lazy || NaN: {
if (type.displayName) {
return type.displayName;
}
const name = displayNameOfNode({ type: type._ctor });
return name ? `React.lazy(${name})` : `lazy`;
}

let's also add tests for an explicit Object.assign(React.lazy(Component), { displayName: 'something' }), as well as a lazy component wrapping something with a displayName, one without, and one with no name at all.

This comment has been minimized.

@chenesan

chenesan Feb 5, 2019

Author Contributor

It's tricky to test if displayName of wrapped component is included in displayName of lazy component. The _ctor is not the wrapped component itself but the function dynamically importing wrapped component. Before it's loaded we can't get the displayName of wrapped component.

(Updated: The wrapped component will be _result after load. I'll replace the _ctor with _result in the suggested change and add tests that replace the _result field of lazy component with the wrapped component, as it's loaded.)

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

interesting, ok - that actually does make sense, considering that one of the use cases for lazy is a component that hasn't been fetched yet.

@@ -537,6 +590,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
const name = type.render.displayName || functionName(type.render);

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

might as well:

Suggested change Beta
const name = type.render.displayName || functionName(type.render);
const name = displayNameOfNode({ type: type.render });
@@ -559,7 +593,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
if (!inst || !this.isValidElement(inst)) {
return false;
}
return typeof inst.type === 'function' || isForwardRef(inst);
return typeof inst.type === 'function' || isForwardRef(inst) || isSuspense(inst);

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member

this is basically isNonHostElement, but let's not worry about the name now

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Feb 5, 2019

Thanks @ljharb . It's ok to start a new round of review now :-)

@ljharb
Copy link
Member

ljharb left a comment

Can we also add a number of debug tests for lazy and Suspense?

@@ -279,6 +316,10 @@ class ReactSixteenAdapter extends EnzymeAdapter {

createMountRenderer(options) {
assertDomAvailable('mount');
assertSuspenseFallbackOptionNotExists(options, 'mount');
if (Object.prototype.hasOwnProperty.call(options, 'suspenseFallback')) {

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member
Suggested change Beta
if (Object.prototype.hasOwnProperty.call(options, 'suspenseFallback')) {
if (has(options, 'suspenseFallback')) {

This comment has been minimized.

@ljharb

ljharb Feb 8, 2019

Member

ping on this one

if (typeof suspenseFallback !== 'undefined' && typeof suspenseFallback !== 'boolean') {
throw TypeError('`options.suspenseFallback` should be boolean or undefined');
}
const shouldReplaceLazyWithFallback = typeof suspenseFallback === 'undefined' ? true : !!suspenseFallback;

This comment has been minimized.

@ljharb

ljharb Feb 5, 2019

Member
Suggested change Beta
const shouldReplaceLazyWithFallback = typeof suspenseFallback === 'undefined' ? true : !!suspenseFallback;
const shouldReplaceLazyWithFallback = typeof suspenseFallback === 'boolean' ? suspenseFallback : true;

altho i wonder if maybe we could invert this, so that "absent" and false are the same?

This comment has been minimized.

@chenesan

chenesan Feb 6, 2019

Author Contributor

That makes sense since now we throw error when it's not boolean or undefined. And I think it's ok to default it to false.


const wrapper = mount(<SuspenseComponent />);

expect(wrapper.debug()).to.equal(`<SuspenseComponent>

This comment has been minimized.

@chenesan

chenesan Feb 6, 2019

Author Contributor

Note that the lazy component node is disappeared in mount when its loaded (_result get loaded and _status = 1). And that will make the .debug() different between shallow and mount when rendering Suspense. I'm not sure if we should also ignore the loaded lazy component node when shallow rendering Suspense to make behavior consistent.

This comment has been minimized.

@ljharb

ljharb Feb 8, 2019

Member

can you elaborate on this?

I would expect that a Suspense is in one of two states: render the children, or render the fallback, and that shallow and mount would be consistent for each of these states.

This comment has been minimized.

@chenesan

chenesan Feb 9, 2019

Author Contributor

For example, the following jsx

const LazyComponent = React.lazy(() => import('WrappedComponent'))

<Suspense fallback={<Fallback />}>
  <LazyComponent />
</Suspense>

in mount, react dev tool shows either fallback when it's not loaded:

<Suspense fallback={...}>
  <Fallback />
</Suspense>

or the wrapped component

<Suspense fallback={...}>
  <WrappedComponent />
</Suspense>

In both cases the LazyComponent node is not shown.

But in shallow with options.suspenseFallback = false we will keep the LazyComponent node there no matter it's loaded or not because (I think) we might want to check if the lazy component node exists. And this makes it inconsistent with mount.

@chenesan

This comment has been minimized.

Copy link
Contributor Author

chenesan commented Feb 6, 2019

Has inverted the suspenseFallback option, added debug tests and also tests for loaded lazy component for mount. @ljharb can review this again now 😄

@@ -50,6 +50,7 @@ describe('<MyComponent />', () => {
- `options.disableLifecycleMethods`: (`Boolean` [optional]): If set to true, `componentDidMount`
is not called on the component, and `componentDidUpdate` is not called after
[`setProps`](ShallowWrapper/setProps.md) and [`setContext`](ShallowWrapper/setContext.md). Default to `false`.
- `options.suspenseFallback`: (`Boolean` [optional]): If set to true, when rendering `Suspense` enzyme will replace all the lazy components in children with `fallback` element prop. Otherwise it won't handle fallback of lazy component. Default to `true`.

This comment has been minimized.

@ljharb

ljharb Feb 8, 2019

Member

is this still correct?

This comment has been minimized.

@chenesan

chenesan Feb 9, 2019

Author Contributor

Ah, yeah, the default is false now.

@@ -279,6 +316,10 @@ class ReactSixteenAdapter extends EnzymeAdapter {

createMountRenderer(options) {
assertDomAvailable('mount');
assertSuspenseFallbackOptionNotExists(options, 'mount');
if (Object.prototype.hasOwnProperty.call(options, 'suspenseFallback')) {

This comment has been minimized.

@ljharb

ljharb Feb 8, 2019

Member

ping on this one

return type.displayName;
}
const name = displayNameOfNode({ type: type._result });
return name ? `React.lazy(${name})` : 'lazy';

This comment has been minimized.

@ljharb

ljharb Feb 8, 2019

Member

how do the dev tools show a lazy component?

This comment has been minimized.

@chenesan

chenesan Feb 9, 2019

Author Contributor

After look into what dev tools show I found it either shows the fallback component or the loaded wrapped component. It won't show the lazy component. But in shallow rendering because we keep the lazy component node, this is still used.


const wrapper = mount(<SuspenseComponent />);

expect(wrapper.debug()).to.equal(`<SuspenseComponent>

This comment has been minimized.

@ljharb

ljharb Feb 8, 2019

Member

can you elaborate on this?

I would expect that a Suspense is in one of two states: render the children, or render the fallback, and that shallow and mount would be consistent for each of these states.

@ljharb ljharb force-pushed the chenesan:react-suspense branch 4 times, most recently from a720e37 to 8e0aa77 Feb 9, 2019

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