-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat: add multipart subscription network adapters for Relay and urql #11301
Conversation
🦋 Changeset detectedLatest commit: 78f3230 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 |
src/utilities/subscriptions/relay.ts
Outdated
}; | ||
const options = generateOptionsForMultipartSubscription(headers || {}); | ||
|
||
return Observable.create((sink) => { |
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.
zen-observable-ts
's Observable
is incompatible with Relay's networking API here, so I'm using the relay-runtime
's Observable
. (Using the former produces a network.execute(...).do is not a function
error.)
A related note about project dependencies: I added @types/relay-runtime
to our dev dependencies, but did not specify relay-runtime
as an optional peer dependency. The risk of confusing a lot of people seemed to outweigh the chance at a mild annoyance if someone were to try to drop in this subscriptions network layer without already having relay-runtime
installed :)
size-limit report 📦
|
src/utilities/subscriptions/urql.ts
Outdated
|
||
const backupFetch = maybe(() => fetch); | ||
|
||
export function createFetchMultipartSubscription( |
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.
I used the same function name here but maybe this should be a more urql-y createMultipartSubscriptionForwarder
Updated the entrypoints and added them to I'm still seeing "risky cross-entry-point nested import" warnings: But when I installed the npm pack-ed library in a project using the urql adapter and look at the generated bundles, they contain what I'd expect/function how I'd expect: Thoughts on whether those warnings are safe to ignore here, @benjamn? |
@alessbell Yes, I think those warnings are safe to ignore here. The main point of the warnings is that code might be duplicated between our internal CommonJS bundles if you reach across the bundles to import nested files, but for these adapters that's not a problem, since consumers will be importing just the code they need, from a single entry point. Also, the warnings are not a big deal if you're using the ESM files rather than CommonJS. |
const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch; | ||
const observerNext = sink.next.bind(sink); | ||
|
||
currentFetch!(uri, options) |
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 don't need to pass in an accept
header here?
(Forget it I'm blind... generateOptionsForMultipartSubscription
)
That's fine in this case - it means that the other files will get inlined in the bundle, which we generally want to avoid - but in this case it's actually a good idea. |
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.
I don't have projects to test it, but I assume you did, and the code looks very reasonable. Let's get this into the next alpha and wait for user feedback.
This PR adds network layer adapters for Relay and urql to be able to consume multipart subscriptions.
Usage looks like this:
Relay
Urql
To start, the API surface is small: users must specify the
uri
and can also provide afetch
override and additionalheaders
.Testing
I've done manual testing with both Relay + urql, and I'd like to write some E2E tests in https://github.com/apollographql/client-router-e2e-tests since it's the integration with the respective frameworks that we care about here. I can open a follow-on PR there if that sounds good.
Documentation
Will do a follow-on PR for documentation.
Checklist: