-
-
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
Support Suspense #1917
Comments
Indeed, Suspense is not yet supported. |
Hey, any idea when Suspense will be supported ? |
@maclockard are you also using |
No, currently just Suspense. Will probably start using lazy as well soon though, but its lower priority to me. |
Any update on this? I'm using Suspense with lazy. Just converted over to using, and getting this error. Wasn't sure it there was something being looked at. Thanks in advance. |
no luck found.. on this simulate event handling.... |
Hello @ljharb I'd like to work on this to make React.Suspense and React.lazy work. Will look into this in the next few days. Since these featrues only works after 16.6, we also have to copy current |
@chenesan for now, just implement it in the 16 adapter; we can iterate in that in the PR. |
For the problem (2) in the PR #1975 comment , after some investigation I think we can just traverse the fiber tree to find all the LazyComponent fiber nodes. Every LazyComponent fiber node hold a And in the // helper function to find all lazy components under a fiber node
function findAllLazyNodes(fiberNode) {
// will return array of LazyComponent fiber node
}
// inside mountRenderer returned by adapter
waitUntilAllLazyComponentsLoaded() {
const lazyNodes = findAllLazyNodes(instance._reactInternalFiber)
// the `_result` might be a promise(when pending), an error(when rejected), module(when resolved)
// will handle all cases in implementaion, now assume there are all promises
const promises = lazyNodes.map(node => node.elementType._result)
await Promise.all(promises)
}
// inside ReactWrapper class
updateUntilAllLazyComponentsLoaded() {
// check if adapter support this
if (typeof this[RENDERER].waitUntilAllLazyComponentsLoaded() !== "function") {
throw Error("Current adapter not support React.lazy")
}
// wait for lazy load, will handle rejected case in adapter
await this[RENDERER].waitUntilAllLazyComponentsLoaded()
// since all of lazy component loaded, force it update to rerender
this.update()
} So we can test the rendering after const LazyComponent = lazy(() => import('/path/to/dynamic/component'));
const Fallback = () => <div />;
const SuspenseComponent = () => (
<Suspense fallback={<Fallback />}>
<LazyComponent />
</Suspense>
);
const wrapper = mount(<SuspenseComponent />)
expect(wrapper.find('Fallback')).to.have.lengthOf(1)
expect(wrapper.find('DynamicComponent')).to.have.lengthOf(0)
await wrapper.waitUntilLazyLoaded()
expect(wrapper.find('Fallback')).to.have.lengthOf(0)
expect(wrapper.find('DynamicComponent')).to.have.lengthOf(1) How do you think about the api @ljharb ? Or anyone could comment on this? |
I think that since Suspense and lazy don’t actually work yet for their intended purpose (bundle splitting) that we don’t have to rush to invent a new API for it. |
@ljharb do you have any info on why lazy doesn't work for code splitting? (I.e. issues etc). I know that Suspense isn't finished, but I thought lazy() worked. Anyhow, I would love a release that just supported rendering the main path of the api so that using enzyme with react 16.6 could work - even though it didn't support testing all variants of Suspense. Maybe that should be the first target? |
Indeed, that’s the first target :-) |
@ljharb Using |
I believe I have misunderstood what is supported in 16.6 :) What I tried to say was that it is quite painful to wait for enzyme to support lazy() and the other 16.6 functionality and I was hoping that could be prioritized before supporting all the general parts of the new Suspense component - i.e. the other usages than lazy loading. If it makes it easier I would think a first version that does not support fallback testing would also help. That said, I'm very much a 👍 on Regards, and sorry for creating confusion. |
@tarjei It also does not yet support server side rendering. |
Thanks for your reply @tarjei ! (1) make enzyme recognize the (2) support waiting lazy loading so we are able to test rendering after lazy loading done (like I've worked on (1) in #1975 (need some polish and tests). After the discussion I might not do (2) for now since it looks not urgent. If someone needs this comment is welcome ;) |
The notion of "waiting" for modules in a test environment doesn't quite make sense to me. So I added support for sync resolving of lazy() in facebook/react#14626. In next version, if you give it a |
any update for a release (be it a major / minor release but stable one) date (tentative one would also do) for the fix of this issue. |
@anushshukla no, see #1975. |
You can create a mock:
|
EDIT: This mock works for FWIW, here is an updated version of @VincentLanglet's mock that also mocks import React from "react";
const react = jest.requireActual("react");
const Suspense = ({ children }) => <div>{children}</div>;
Suspense.displayName = "Suspense";
const lazy = React.lazy(() =>
Promise.resolve().then(() => ({
default() {
return null;
}
}))
);
lazy.displayName = "lazy";
module.exports = { ...react, Suspense, lazy }; |
@thefivetoes When I use you
Does it work for you ? This error seems to be normal since I mock the |
@VincentLanglet you are correct, my bad – I am only doing |
just wanted to thank you guys, @VincentLanglet and @thefivetoes, in writing, for the help. Also wanted to share that in test cases of components who have nested components using Suspense + Lazy, you can do the following
In my case, AsyncComponent/index.js which uses Suspense + Lazy AsyncComponent/mocks/index.js which is an alternate to Suspense + Lazy In the failing test file, just added the below line,
Also, we are utilising React test renderer other than enzyme and would suggest the same to others that not to rely on one testing utility while we all wish to have that ideal one but reality is this. |
@thefivetoes did you do anything special to get that mock off the ground? No matter what I do, I end up with the following exception:
I tried with a custom webpack + babel config as well as ejecting from the latest create-react-app (plus bumping the react + enzyme versions). EDIT: Removing the initial import allows the mock to actually fire, but the mocked lazy implementation doesn't seem to match the actual lazy implementation. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
v3.10.0 has now been released. |
@thefivetoes How to make this work on mount? Can you please help? |
Not sure if @ljharb (Jordan) is still getting replies on this thread so I'll tag him directly (sorry, Jordan if you already are!) ❤️ |
@rodoabad i am, but haven't had time to get to all of my hundreds of notifications yet. please file a new issue, since code coverage of React.lazy isn't the same as overall Suspense support. @shridharkalagi similarly, if you're still having an issue, please file a new issue. |
Quoting #1917 (comment)
@ljharb given that, do you think that https://github.com/airbnb/babel-plugin-dynamic-import-node could support sync thenable mode? That is, transforming |
No, I don't - that would violate the spec (altho an older version of that transform did operate that way; you could always peg to that version). Either way, forcing a babel transform so that enzyme works properly both won't work in browsers and doesn't seem ideal. |
OK I see, I just wonder how can I actually benefit from facebook/react#14626 |
I tried with |
For coverage propose, I've done the following code
|
I needed to test my lazy component using Enzyme. Following approach worked for me to test on component loading completion:
Test Code ->
This is hacky but working well for Enzyme library. |
@ShailyAggarwalK I'm not sure what this example means seeing as React.lazy does not return something with a The result of running this code in any normal environment would be something like
|
Current behavior
I'm currently getting the following error using a Suspense component in my tests.
Looking at the current react work tags, this is for the suspense component: https://github.com/facebook/react/blob/v16.6.0/packages/shared/ReactWorkTags.js#L44
Looking at the current implementation of
detectFiberTag
it doesn't look for Suspense components: https://github.com/airbnb/enzyme/blob/enzyme%403.7.0/packages/enzyme-adapter-react-16/src/detectFiberTags.js#L42I believe the exception is coming here: https://github.com/airbnb/enzyme/blob/enzyme%403.7.0/packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js#L103 after it hits the default for the switch.
Expected behavior
Can handle suspense components
API
Version
Adapter
The text was updated successfully, but these errors were encountered: