refactor: use Promise constructor to wrap builtin gzip #7229

dnalborczyk · 2022-12-07T02:25:53Z

support for Cloudflare Workers

netlify · 2022-12-07T02:26:02Z

❌ Deploy Preview for apollo-server-docs failed.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`0154317`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-server-docs/deploys/63910c41d759780008ecd2c1

codesandbox-ci · 2022-12-07T02:26:45Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0154317:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

glasser · 2022-12-07T21:54:58Z

I am happy to merge this change because there's nothing particularly negative about it and I trust you that it resolves the CF worker issue.

But it is hard for me to know as a maintainer what to avoid doing in the future — eg, the fact that zlib is fine for CF workers and utils is not seems unpredictable. If you have any thoughts on something straightforward we can add to our CI process to check that we're not breaking that, that would be awesome — I don't think you suggested anything specific on #6034 but I might have missed it.

@glasser

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-plugin-response-cache@4.1.0 ### Minor Changes - [#7241](#7241) [`d7e9b9759`](d7e9b97) Thanks [@glasser](https://github.com/glasser)! - If the cache you provide to the `cache` option is created with `PrefixingKeyValueCache.cacheDangerouslyDoesNotNeedPrefixesForIsolation` (new in `@apollo/utils.keyvaluecache@2.1.0`), the `fqc:` prefix will not be added to cache keys. ## @apollo/server@4.3.0 ### Minor Changes - [#7241](#7241) [`d7e9b9759`](d7e9b97) Thanks [@glasser](https://github.com/glasser)! - If the cache you provide to the `persistedQueries.cache` option is created with `PrefixingKeyValueCache.cacheDangerouslyDoesNotNeedPrefixesForIsolation` (new in `@apollo/utils.keyvaluecache@2.1.0`), the `apq:` prefix will not be added to cache keys. Providing such a cache to `new ApolloServer()` throws an error. ### Patch Changes - [#7232](#7232) [`3a4823e0d`](3a4823e) Thanks [@glasser](https://github.com/glasser)! - Refactor the implementation of `ApolloServerPluginDrainHttpServer`'s grace period. This is intended to be a no-op. - [#7229](#7229) [`d057e2ffc`](d057e2f) Thanks [@dnalborczyk](https://github.com/dnalborczyk)! - Improve compatibility with Cloudflare workers by avoiding the use of the Node `util` package. This change is intended to be a no-op. - [#7228](#7228) [`f97e55304`](f97e553) Thanks [@dnalborczyk](https://github.com/dnalborczyk)! - Improve compatibility with Cloudflare workers by avoiding the use of the Node `url` package. This change is intended to be a no-op. - [#7241](#7241) [`d7e9b9759`](d7e9b97) Thanks [@glasser](https://github.com/glasser)! - For ease of upgrade from the recommended configuration of Apollo Server v3.9+, you can now pass `new ApolloServer({ cache: 'bounded' })`, which is equivalent to not providing the `cache` option (as a bounded cache is now the default in AS4). ## @apollo/server-integration-testsuite@4.3.0 ### Patch Changes - [#7228](#7228) [`f97e55304`](f97e553) Thanks [@dnalborczyk](https://github.com/dnalborczyk)! - Improve compatibility with Cloudflare workers by avoiding the use of the Node `url` package. This change is intended to be a no-op. - Updated dependencies \[[`3a4823e0d`](3a4823e), [`d057e2ffc`](d057e2f), [`f97e55304`](f97e553), [`d7e9b9759`](d7e9b97), [`d7e9b9759`](d7e9b97)]: - @apollo/server@4.3.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

dnalborczyk · 2022-12-13T01:58:00Z

thank you @glasser for pulling this in!

I am happy to merge this change because there's nothing particularly negative about it and I trust you that it resolves the CF worker issue.

But it is hard for me to know as a maintainer what to avoid doing in the future — eg, the fact that zlib is fine for CF workers and utils is not seems unpredictable.

I agree, that's totally understandable. Cloudflare Workers are implemented using the v8 engine (always latest as far as I understand) in addition to Web API (URL, URLSearchParams, URLPattern, TextEncoder etc.). Wrangler is the build and deployment tool for CFW. as of v2, they adopted esbuild as default bundler, for the fantastic performance, as CFW currently only supports a single file to be deployed. however developers can use any bundler (webpack, rollup, etc.). esbuild transforms js/ts, bundles, and minifies - but it doesn't know anything about any node specific built-ins (util, assert etc.) nor globals (Buffer, process etc.). Those can be, and are, polyfilled by plugins. if the plugins were up-to-date and fully working none of this would be a problem, unfortunately they are not, as esbuild is also a fairly new project.

If you have any thoughts on something straightforward we can add to our CI process to check that we're not breaking that, that would be awesome — I don't think you suggested anything specific on #6034 but I might have missed it.

Good point. There might be not a straight forward way to easily accomplish that without constraining further development and improvements of Apollo Server. Generally speaking, preferring JS/Web API over platform specific API would definitely help, for CFW or any other platform than node.js, such as deno, bun etc. In addition, avoiding (currently) non-polyfillable modules such as http, fs etc. would help as well.

I would think most developers who are using CFW are hopefully aware of the implications the platform comes with, and we could (or even should) add some additional suggestions on the the CFW integration docs, such as pinning the deps, and always use lock files etc., otherwise bring it up in an issue or a PR.

glasser · 2022-12-13T02:08:48Z

Thanks. I guess my question is: is there something pretty simple we can do in a smoke test perhaps using Wrangler directly that builds a minimal Apollo Server app and sees that it at least starts up properly (ie the imports don't fail)?

dnalborczyk · 2022-12-14T02:27:04Z

is there something pretty simple we can do in a smoke test perhaps using Wrangler directly that builds a minimal Apollo Server app and sees that it at least starts up properly (ie the imports don't fail)?

yes, there is! for local development wrangler is using miniflare under the hood, which is (or better has been) running on node.js, which probably wouldn't help much. miniflare v3 is switching from node.js to the open sourced workerd runtime, which itself is powering cloudflare workers, which should behave pretty much exactly as the published CFW version.

unfortunately, the problem is that the generated output and the behavior really depends on which tools and which versions are being used 😞

glasser · 2022-12-14T03:41:16Z

OK, so waiting til v3 might make sense for this sort of testing?

What's the next step we can take towards getting Cloudflare support on our integrations page? Your package ready for that?

dnalborczyk · 2022-12-16T18:10:19Z

OK, so waiting til v3 might make sense for this sort of testing?

forgot to mention, wrangler v2 has support for an experimental-local flag, which then uses miniflare v3 powered by workerd. the last couple days I got everything working locally: CFW standalone as well as federation (gateway + subgraphs).

https://github.com/dnalborczyk/apollo-server-cloudflare-workers
https://github.com/dnalborczyk/apollo-federation-cloudflare-workers

npm start for local development, might not currently work with node v19 and/or npm v9. I haven't looked into the details.

What's the next step we can take towards getting Cloudflare support on our integrations page? Your package ready for that?

There's only a couple things left, other than that it should be ready to go, we can move the repo over to as integrations:

I removed the path and http method restriction dnalborczyk/apollo-server-integrations-cloudflare-workers@8859f7c because it did only support POST requests (no GET), and a path was required (should be optional). I want to re-introduce both again at a later time. (will be optional and non-breaking).

another small thing on my todo list is to look at: https://github.com/dnalborczyk/apollo-server-integrations-cloudflare-workers/blob/97c085d7e481a94d668a0b4378d7e9462a3e36d7/src/startServerAndCreateCloudflareWorkersHandler.ts#L97

last but not least we need to add some tests.

refactor: use Promise constructor to wrap builtin gzip

c83f919

dnalborczyk marked this pull request as ready for review December 7, 2022 02:39

alessbell mentioned this pull request Dec 7, 2022

Introduce Changesets apollographql/apollo-client#10337

Merged

glasser mentioned this pull request Dec 7, 2022

Call for maintainers: apollo-server-cloudflare #6034

Closed

changeset

0154317

glasser enabled auto-merge (squash) December 7, 2022 21:57

glasser merged commit d057e2f into apollographql:main Dec 7, 2022

This was referenced Dec 7, 2022

Version Packages #7231

Merged

Version Packages boost-entropy-typescript/apollo-server#108

Open

Version Packages SimenB/apollo-server#298

Closed

dnalborczyk deleted the gzip/promise branch December 13, 2022 01:04

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use Promise constructor to wrap builtin gzip #7229

refactor: use Promise constructor to wrap builtin gzip #7229

dnalborczyk commented Dec 7, 2022 •

edited

Loading

netlify bot commented Dec 7, 2022 •

edited

Loading

codesandbox-ci bot commented Dec 7, 2022 •

edited

Loading

glasser commented Dec 7, 2022

dnalborczyk commented Dec 13, 2022

glasser commented Dec 13, 2022

dnalborczyk commented Dec 14, 2022 •

edited

Loading

glasser commented Dec 14, 2022

dnalborczyk commented Dec 16, 2022

refactor: use Promise constructor to wrap builtin gzip #7229

refactor: use Promise constructor to wrap builtin gzip #7229

Conversation

dnalborczyk commented Dec 7, 2022 • edited Loading

netlify bot commented Dec 7, 2022 • edited Loading

❌ Deploy Preview for apollo-server-docs failed.

codesandbox-ci bot commented Dec 7, 2022 • edited Loading

glasser commented Dec 7, 2022

dnalborczyk commented Dec 13, 2022

glasser commented Dec 13, 2022

dnalborczyk commented Dec 14, 2022 • edited Loading

glasser commented Dec 14, 2022

dnalborczyk commented Dec 16, 2022

dnalborczyk commented Dec 7, 2022 •

edited

Loading

netlify bot commented Dec 7, 2022 •

edited

Loading

codesandbox-ci bot commented Dec 7, 2022 •

edited

Loading

dnalborczyk commented Dec 14, 2022 •

edited

Loading