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.

Sign up for GitHub

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

Jump to bottom

fix: change use of instanceof Promise to use is-promise library #1837

Merged
merged 7 commits into from Aug 10, 2020
Merged

fix: change use of instanceof Promise to use is-promise library #1837

merged 7 commits into from Aug 10, 2020

Conversation

cajames
Copy link
Contributor

@cajames cajames commented Jul 28, 2020 •
edited

Closes #1836

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@cajames cajames changed the title fix: wait for promise resolution before returning fix(delegateRequest): wait for promise resolution before returning Jul 28, 2020
@ardatan
Copy link
Owner

ardatan commented Jul 28, 2020 •
edited

Thank you for opening this PR but this breaks sync execution because executor can be sync as well. Instead of instanceof Promise, we can use 'then' in executionResult.

@cajames
Copy link
Contributor Author

cajames commented Jul 28, 2020

@ardatan okay I can change the implementation to check the object then.

@yaacovCR
Copy link
Collaborator

This is not a fix, but rather a change of behavior. The linked issue you provided suggested replacing instanceof with the is-promise package. Does that work? If so, it should be used across entire package.

@cajames
Copy link
Contributor Author

cajames commented Jul 28, 2020 •
edited

we can use 'then' in executionResult

@ardatan yes that's one way. There's a more thorough check. (linked below)

This is not a fix, but rather a change of behavior.

Ah, I didn't realise. Thanks for clarifying

The linked issue you provided suggested replacing instanceof with the is-promise package. Does that work? If so, it should be used across entire package.

@yaacovCR yes I can confirm that using that package works too. It's a one-line function, which could be added to this packages utils.

https://github.com/then/is-promise/blob/1d547300e780a4eca391236f15e5ccbf76a5789d/index.js#L4-L6

I was thinking of just implementing this directly in code. If instanceof Promise is used around the codebase, then we can place this function as a util, and use it where required.

What would you prefer, use the is-promise library, or add it as a util function? I can update the PR accordingly.

@cajames
Copy link
Contributor Author

cajames commented Jul 28, 2020 •
edited

@yaacovCR you're right, there are quite a few instances of instanceof Promise in the codebase (delegate, links, stitch and wrap packages). Would be best to refactor all of them in this.

@cajames cajames changed the title fix(delegateRequest): wait for promise resolution before returning fix: change use of instanceof Promise to use is-promise library Jul 28, 2020
@yaacovCR
Copy link
Collaborator

yaacovCR commented Jul 28, 2020 via email

@yaacovCR
Copy link
Collaborator

yaacovCR commented Jul 28, 2020 via email

@cajames
Copy link
Contributor Author

cajames commented Jul 29, 2020

@ardatan @yaacovCR I've updated the PR to change all the uses of instanceof Promise. Would you be okay with this? Or would you like me to limit this change to just this one delegate package? I've noticed the only other use case in src-code is in the Upload module. The other cases are just in test fixtures.

@ardatan
Copy link
Owner

ardatan commented Jul 29, 2020

@cajames I think we can merge it! Could you fix merge conflicts?

@ardatan ardatan added the bugfix label Jul 29, 2020
@cajames
Copy link
Contributor Author

cajames commented Jul 29, 2020

@ardatan sweet! And yep I've fixed the conflicts.

@yaacovCR yaacovCR force-pushed the master branch 3 times, most recently from ffb642b to 7089fa6 Compare July 31, 2020 05:02
@yaacovCR
Copy link
Collaborator

yaacovCR commented Aug 4, 2020

Defer to @ardatan, but my two cents is to fix in code only and not tests or to inline similar version of function as graphql-js

@cajames
Copy link
Contributor Author

cajames commented Aug 7, 2020 •
edited

@yaacovCR Yes fair. You'll notice I've reverted all test changes, so no longer changing any tests. It's just logic now. Happy to inline the function if that's preferred.

Hey @ardatan, is there anything blocking this fix from being merged?

@wenisman
Copy link

@yaacovCR Yes fair. You'll notice I've reverted all test changes, so no longer changing any tests. It's just logic now. Happy to inline the function if that's preferred.

Hey @ardatan, is there anything blocking this fix from being merged?

Hey guys,
this fix is necessary for our systems too. Is there a time frame when this can be merged?

@ardatan
Copy link
Owner

ardatan commented Aug 10, 2020

Thanks @cajames !

@ardatan ardatan merged commit a19b809 into ardatan:master Aug 10, 2020
@theguild-bot
Copy link
Collaborator

The latest changes of this PR are available as alpha in npm: 6.0.17-alpha-a19b8093.0

Quickly update your package.json by running:

npx match-version @graphql-tools 6.0.17-alpha-a19b8093.0

@cajames cajames deleted the fix-remote-executor branch August 10, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

delegateRequest not waiting for Remote Executor to resolve
5 participants
@cajames @ardatan @yaacovCR @wenisman @theguild-bot