Skip to content

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 10, 2022

Deprecates the positional arguments to createSourceEventStream, to be removed in the next major version, in favor of named arguments.

Motivation:

  1. aligns createSourceEventStream with the other exported entrypoints graphql, execute, and subscribe
  2. allows simplification of mapSourceToResponse

suggested by @IvanGoncharov

Deprecates the positional arguments to `createSourceEventStream`, to be removed in the next major version.
@netlify
Copy link

netlify bot commented Jun 10, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit f5ebc6f
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62a5f00e7962650009f1d1a2
😎 Deploy Preview https://deploy-preview-3634--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR yaacovCR changed the title createSourceEventStream: allow named arguments createSourceEventStream: introduce named arguments and deprecate positional arguments Jun 10, 2022
@github-actions
Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR yaacovCR requested a review from a team June 10, 2022 04:11
@yaacovCR
Copy link
Contributor Author

To be backported to v16 with later PR to remove positional arguments in v17

@yaacovCR yaacovCR added the PR: feature 🚀 requires increase of "minor" version number label Jun 10, 2022
yaacovCR added 2 commits June 10, 2022 11:54
See open TS issue microsoft/TypeScript#31613

Marked with FIXME for when TS feature/bug fix lands upstream

[This code will likely be removed in v17 prior to the feature landing, but may be retained within v16 for longer.]

Similar code "works" at https://github.com/graphql/graphql-js/blob/1f8ba95c662118452bd969c6b26ba4e9050c55da/src/error/GraphQLError.ts#L47 because all the option properties are optional.
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@yaacovCR yaacovCR merged commit db643e9 into graphql:main Jun 12, 2022
@yaacovCR yaacovCR deleted the named branch June 12, 2022 13:57
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Jun 14, 2022
…tional arguments

BACKPORT OF graphql#3634

Deprecates the positional arguments to createSourceEventStream, to be removed in the next major version, in favor of named arguments.

Motivation:

1. aligns createSourceEventStream with the other exported entrypoints graphql, execute, and subscribe
2. allows simplification of mapSourceToResponse

suggested by @IvanGoncharov
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Jun 14, 2022
…tional arguments

BACKPORT OF graphql#3634

Deprecates the positional arguments to createSourceEventStream, to be removed in the next major version, in favor of named arguments.

Motivation:

1. aligns createSourceEventStream with the other exported entrypoints graphql, execute, and subscribe
2. allows simplification of mapSourceToResponse

suggested by @IvanGoncharov
IvanGoncharov pushed a commit that referenced this pull request Jun 15, 2022
…tional arguments (#3645)

BACKPORT OF #3634

Deprecates the positional arguments to createSourceEventStream, to be removed in the next major version, in favor of named arguments.

Motivation:

1. aligns createSourceEventStream with the other exported entrypoints graphql, execute, and subscribe
2. allows simplification of mapSourceToResponse

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

Labels

PR: feature 🚀 requires increase of "minor" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants