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

fix: make graphql-ws not optional #111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Myshkouski
Copy link

@Myshkouski Myshkouski commented Mar 1, 2025

Replaces #79 and fixes graphql-ws import issue.

@Myshkouski
Copy link
Author

@tobiasdiez Could you explain why graphql-ws was added as optional in 6a7b9c2?

@Myshkouski Myshkouski marked this pull request as ready for review March 1, 2025 16:15
@tobiasdiez
Copy link
Collaborator

Thanks for your PR. Most graphql servers (definitely apollo) have websockets as an optional feature that needs to be installed via a new package. I've oriented myself at this standard procedure and thus would like to have graphql-ws as an optional dependency, that you can install if you need ws support.

Could you please try #79 and see if it fits your needs?

@Myshkouski
Copy link
Author

Myshkouski commented Mar 2, 2025

Could you please try #79 and see if it fits your needs?

I think await import() would not resolve the issue, and there are some reasons why:

1. Running with runtime other than NodeJS will fail

h3 should be runtime agnostic, so @as-integrations/h3 should support runtimes from the adapters list.
With Deno, package management does not use node_modules by default, so @as-integrations/h3 will be imported from npm registry or global cache directly. Since package.json defines graphql-ws as optional, Deno will not install it as a dependency.

So that, managing dependencies by using node_modules dir is not platform agnostic as h3 require.

2. Code bundling will fail in downstream projects

Downstream projects may use code bundlers to produce single file with all dependencies' code included. In this case, bundler analyzes @as-integrations/h3 code and tries to import graphql-ws (with either static or dynamic import), event when developer not intended to use webscket-related features.
Nitro uses that build workflow to deploy to some serverless environments. In my case, Cloudflare Workers and Deno Deploy fail to import graphql-ws at runtime.

So that, both import { ... } from 'graphql-ws' and await import('graphql-ws') will cause problems in downstream projects when using bundlers.

I fully understand that you want to provide websocket-related features as "optional", but seems managing node_modules dir and peerDependenciesMeta is not a correct approach. But what should we do?

There are 2 steps:

1. Remove peerDependenciesMeta.

This will make the current major version compatible with different package managers.

2. Extract websocket-related features to separate ES module in next major version.

Developers should decide what to import from the package:

  • For core features - import { startServerAndCreateH3Handler } from "@as-integrations/h3".
  • For websocket features - import { defineGraphqlWebSocketHandler } from "@as-integrations/h3/websocket". This would ensure importing only what project actually requires.

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.

2 participants