-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
Fix instance where renderToStringAsync returns a promise of a promise #378
Conversation
I want to test this in my project to see if this change fixes it. All being well, I'll then add a test here and submit it for review
I'm unclear why the previous didn't seem to work for me
So far no luck actually reproducing the issue. I will take a different tact and remove code from my app until the issue goes away, to figure out what causes the behaviour.
🦋 Changeset detectedLatest commit: 7bd6b29 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
test/compat/async.test.jsx
Outdated
@@ -175,4 +175,46 @@ describe('Async renderToString', () => { | |||
const rendered = await promise; | |||
expect(rendered).to.equal('<p>ok</p>'); | |||
}); | |||
|
|||
it('should render JSX after a urql component', async () => { |
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 might also want to try a test where it's triple-y nested as it might warrant a loop similar to the Array.isArray
one.
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 do you think this would look like?
If I do something like:
const Context = createContext();
let c = 0;
const Fetcher = ({ children }) => {
c++;
if (c === 1) {
throw Promise.resolve();
}
return <Fragment>{children}</Fragment>;
};
const LazyComponent = lazy(
async () =>
function ImportedComponent() {
return <div>2</div>;
}
);
const NestedLazyComponent = lazy(
async () =>
function ImportedComponent() {
return (
<Context.Provider>
<Fetcher>
<LoadableComponent />
</Fetcher>
</Context.Provider>
);
}
);
const LoadableComponent = ({}) => (
<Suspense fallback={'...loading'}>
<LazyComponent />
</Suspense>
);
const NestedLoadableComponent = ({}) => (
<Suspense fallback={'...loading'}>
<NestedLazyComponent />
</Suspense>
);
const rendered = await renderToStringAsync(
<Context.Provider>
<Fetcher>
<NestedLoadableComponent />
</Fetcher>
</Context.Provider>
);
expect(rendered).to.equal(`<div>2</div>`);
Then the current solution continues to work
But I still need the provider
createSuspender(); | ||
const { Suspender: SuspenderTwo, suspended: suspendedTwo } = | ||
createSuspender(); | ||
const { |
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 git commit formatter hook makes these changes. Let me know if you want me to keep them as is.
Have you tried breaking it by increasing the nesting even further? Just wondering 😅 |
Fixes #377
I have had this happen with a project I work on. This includes a minimal test reproduction of it.