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

fix: correct maybe deep promise typing #906

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shreyas44
Copy link
Contributor

@shreyas44 shreyas44 commented May 15, 2021

Removing the conditional type for null in MaybePromiseDeep seems to fix the below linked issue without removing any type safety.
I've tested it with the getters for edges and pageInfo with and without promises.

However, I feel that conditional type was there for a reason, but I couldn't figure out why and I'm not entirely sure if this breaks something else as it seems to be a utility used in multiple places across the library.

Closes #853

@shreyas44 shreyas44 changed the title Fix maybe deep promise type fix: correct maybe deep promise typing May 15, 2021
@jasonkuhrt
Copy link
Contributor

Is the change in this PR causing https://github.com/graphql-nexus/nexus/pull/906/checks?check_run_id=2589178499#step:6:12? Would seem so but not sure.

@shreyas44
Copy link
Contributor Author

Yep, you're right, the change in the PR did cause the issue. It seems to stem from the fact that TypeScript distributes unions when evaluating conditional types.

I fixed that and also removed a redundant MaybePromise type from the return type of resolve as MaybePromiseDeep returns a MaybePromise itself.

@jasonkuhrt
Copy link
Contributor

Tests still seem to be failing though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow promises to be returned in a field in the resolver of connection
3 participants