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

separate transport-specific streaming logic out #189

Merged
merged 27 commits into from
Feb 19, 2024
Merged

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Feb 5, 2024

See a lot of comments further down :)

@phryneas

This comment was marked as outdated.

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

2 similar comments
@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

@phryneas

This comment was marked as outdated.

Copy link
Contributor

github-actions bot commented Feb 8, 2024

A new release has been made for this PR. You can install it with npm i @apollo/experimental-nextjs-app-support@0.0.0-commit-fac6765.

@phryneas
Copy link
Member Author

phryneas commented Feb 8, 2024

/release:pr

Copy link
Contributor

github-actions bot commented Feb 8, 2024

A new release has been made for this PR. You can install it with npm i @apollo/experimental-nextjs-app-support@0.0.0-commit-c266287.

Copy link
Member Author

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

This should be in a good state to be reviewed.

In a second PR, I'm gonna split this up into multiple actual packages, but this is more than enough for one PR.

Comment on lines +1 to +4
export * from "./rsc/index.js";
export * from "./ssr/index.js";

export {};
Copy link
Member Author

Choose a reason for hiding this comment

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

This only exists for TypeScript types - I can't export types * here, as from a TypeScript perspective we are actually importing values from this file, although the js artefact here is never used.
After splitting up the packages, I'll make sure that the artefact is deleted, but right now this bundling detail doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth adding this as a code comment? I know I'm going to forget this in 6 months 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! I added a comment over in #193

Comment on lines 91 to +107
"typesVersions": {
"*": {
"ssr": [
"./dist/ssr/index.d.ts"
"./dist/ssr/next.ssr.d.ts"
],
"rsc": [
"./dist/rsc/index.d.ts"
],
"manual": [
"./dist/ssr/manual.d.ts"
],
"core": [
"./dist/combined.d.ts"
]
}
},
"typings": "./dist/combined.d.ts",
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the types that will actually be picked up by TypeScript, since Next.js projects use moduleResolution: node in their tsconfig.json, although node16 would be more correct (and pick up theexports above).

import type { ApolloClient } from "@apollo/client";
import * as React from "react";
import type { ApolloClient } from "@apollo/client/index.js";
import { cache } from "react";
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: for this to work with rehackt instead (which would be more consistent), rehackt's RSC bundling needs to be slightly modified.

A fight for another day.

Copy link
Member

Choose a reason for hiding this comment

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

Was this the 0.5.0 release over there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but I also drop out rehackt as a dependency for this in the next PR :)

Comment on lines +18 to +20
const buildManualDataTransportSSRImpl = ({
useInsertHtml,
}: BuildArgs): DataTransportProviderImplementation<HydrationContextOptions> =>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the abstraction that will be shared by Next.js, Redwood and probably any other framework until React adds some primitives for this.

});
}

registerDispatchRequestStarted!((options: WatchQueryOptions) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These can be !ed with confidence since they will always be passed in in the SSR build - they will just not be present in the browser build, but this is the SSR implementation of ManualDataTransport

Comment on lines +12 to +18
const insertHtml = useContext(ServerInsertedHTMLContext);
if (!insertHtml) {
throw new Error(
"ApolloNextAppProvider cannot be used outside of the Next App Router!"
);
}
return insertHtml;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is all the code that's truly Next.js specific.

@phryneas
Copy link
Member Author

phryneas commented Feb 9, 2024

/release:pr

@phryneas phryneas marked this pull request as ready for review February 9, 2024 17:07
@phryneas phryneas requested a review from a team as a code owner February 9, 2024 17:07
Copy link
Contributor

github-actions bot commented Feb 9, 2024

A new release has been made for this PR. You can install it with npm i @apollo/experimental-nextjs-app-support@0.0.0-commit-b271305.

@phryneas phryneas changed the title separate transport-specific streaming logic out into separate packages separate transport-specific streaming logic out Feb 12, 2024
@luchillo17
Copy link

luchillo17 commented Feb 13, 2024

I've started checking out these changes and there's a lot to look at, but I see the description of the PR is very small, it would be nice to know what is the reason this PR exists, the title doesn't tell me much, is it a refactor for DRY principles? or is there a functional change in how data and/or UI are streamed to the client side?

@phryneas
Copy link
Member Author

phryneas commented Feb 13, 2024

@luchillo17 We are in the process of separating this package into two separate ones - one more generic @apollo/client-react-streaming and the more specific @apollo/experimental-nextjs-app-support. This is necessary because other frameworks (e.g. RedwoodJs) are now adapting streaming SSR, and we want to add easier support for those.

This is the first of multiple PRs, and the follow-up PR with the actual package split is going over in #193

Honestly, I get that this is a lot, and you really don't need to review all of it - but it would be very helpful if you could give the feedback if jest keeps working for you as this progresses along :)

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

I'm not going to pretend I understand all of this 100% but I've learned a lot looking through this. I like a lot of the refactoring that has been done here and think this is a solid base moving forward. I've held this up long enough though, so I'll continue to dig deeper on my own! Incredible work 🎉

},
"./manual": {
"require": {
"types": "./dist/manual.ssr.d.cts",
Copy link
Member

Choose a reason for hiding this comment

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

TIL there is a .cts extension

import type { ApolloClient } from "@apollo/client";
import * as React from "react";
import type { ApolloClient } from "@apollo/client/index.js";
import { cache } from "react";
Copy link
Member

Choose a reason for hiding this comment

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

Was this the 0.5.0 release over there?

results: [],
error: undefined,
complete: false,
});

vi.advanceTimersByTime(100);
mock.timers.tick(100);
Copy link
Member

Choose a reason for hiding this comment

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

Neat! I didn't realize node had built in mock timers

}
if (id in store) value = store[id] as T;
}
const dataTransport = useContext(DataTransportContext);
Copy link
Member

Choose a reason for hiding this comment

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

I like this refactor 💯

package/src/ssr/Manual/ManualDataTransport.tsx Outdated Show resolved Hide resolved
package/src/ssr/WrappedInMemoryCache.tsx Outdated Show resolved Hide resolved

assert.ok(attemptedRenderCount > 0);
assert.ok(finishedRenderCount > 0);
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if this is a naive question, but does the cache play any role here? As in, do we need to check the cache to make sure the result made it in the cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't hurt - I've added another assertion :)

@phryneas phryneas merged commit 7c2ca9e into main Feb 19, 2024
9 checks passed
@phryneas phryneas deleted the pr/generalized-package branch February 19, 2024 10:58
@dac09
Copy link

dac09 commented Feb 19, 2024

🥳🥳🥳🥳🥳

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.

4 participants