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

Make it work on edge functions #1440

Open
ramiel opened this issue Apr 3, 2023 · 19 comments
Open

Make it work on edge functions #1440

ramiel opened this issue Apr 3, 2023 · 19 comments
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request

Comments

@ramiel
Copy link

ramiel commented Apr 3, 2023

Is your feature request related to a problem? Please describe.
typegraphql cannot work, at the moment, on edge function engines, like the one of cloudflare workers or vercel@edge. The only reason why it cannot work is because the "emit schema to file" feature relies on path and fs libraries. No other part of the code is problematic. This is a little shame because a secondary feature is preventing it to work.

Describe the solution you'd like
Since the only feature is emitSchemaFile we could isolate this functionality into a different package (e.g. typegraphql-schema-emit or as submodule typegraphql/schema-emit) and have it as optional peer dependency, so that, when the user want to use the non programmatic way of emitting schema they need that dependency. As well, when they want to use the programmatic way, they need to referthat dependency directly.

Describe alternatives you've considered
It's not easy to isolate some parts of a library. Maybe tree shaking can help but it's not easy.

@MichalLytek MichalLytek added the Question ❔ Not future request, proposal or bug issue label Apr 3, 2023
@MichalLytek
Copy link
Owner

Have you tried bundling the app and configuring webpack/other bundler to use dummy mock instead of node.js packages?

Something like this for browser:
https://github.com/MichalLytek/type-graphql/blob/master/examples/apollo-client/package.json#L19-L23

@ramiel
Copy link
Author

ramiel commented Apr 3, 2023

I try to see if this is possible with vercel@edge where you actually have no control of the bundler

@ramiel
Copy link
Author

ramiel commented Apr 3, 2023

it looks like there's no similar way or, at the least, I wasn't able to find one

@MichalLytek
Copy link
Owner

MichalLytek commented Apr 3, 2023

I mean you should bundle your project by yourself (in order to mock node.js api) and then publish the bundle to vercel edge

@ramiel
Copy link
Author

ramiel commented Apr 3, 2023

I see. Definitely harder but doable. Thanks

@Enalmada
Copy link

Enalmada commented Jul 8, 2023

I just finished building a Next.js project with with type-graphql 2.0.0-beta.2 and love it so much. Unfortunately when trying to run my graphql route in export const runtime = 'edge'; mode in preparation to deploy to cloudflare I am getting Module not found: Can't resolve 'path'

@MichalLytek Would making emitSchemaFile code dynamically import/require fs/path libraries only when needed fix the situation?
const path = await import("path"); before use rather than import path from "path";

I suspect if emitSchemaFile is false then the code path would hopefully not be triggered on edge builds and this could be a way forward.

Full error

./node_modules/.pnpm/type-graphql@2.0.0-beta.2_class-validator@0.14.0_graphql@16.7.1/node_modules/type-graphql/dist/helpers/filesystem.js:5:39
Module not found: Can't resolve 'path'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/.pnpm/type-graphql@2.0.0-beta.2_class-validator@0.14.0_graphql@16.7.1/node_modules/type-graphql/dist/utils/emitSchemaDefinitionFile.js

@ramiel
Copy link
Author

ramiel commented Sep 8, 2023

@Enalmada as a temporary workaround I forked the project and created the package type-graphql-edge that runs on edge environment. The option and the logic to emit schema file is removed entirely.
Another issue for the edge environment is the usage of global that has been removed.

@ramiel
Copy link
Author

ramiel commented Sep 28, 2023

I tried several approach to this and trying to alias the node libraries is too hard to make it reasonable. It only works in very simple setup, but for example in mine I have a separated prisma project that uses type-grapqhl-prisma to generate code which introduce other problems. Also, type-grapqhl relies for some reason on global variable that is not present in the edge runtime. Since the blocking issue are simpler to solve in this library more than in the code that uses it, would you accept any PR that goes in the direction of supporting the edge runtime? This of course would involve having a separate package for writing the schema to a file. I don't know if this is in the interest of this library so let me know if it's worth to go in this direction with a pull request.

Also, this would allow using this library with prisma accelerate that instead runs on edge runtime.

@akhdefault
Copy link

Any progress on that? Would be awesome to run it with CF workers.

@ramiel
Copy link
Author

ramiel commented Oct 3, 2023

@akhdefault I'm only going to work on it if this is the direction the maintainer of the library wants to pursuit.

@MichalLytek
Copy link
Owner

@ramiel
There are no easy ways to fix that:

  • making await import means we can't have sync buildSchema anymore
  • going with subpath or separate package approach means users lose the automatic schema emitting feature emitSchemaFile: true
  • global has to stay (maybe as a globalThis) as it's the way to prevent issues with multiple metadata storages, when you have your type definitions separated from the runtime/server part, as there's a risk of multiple version/instances of type-graphql in node_modules

In ideal world we would have @typegraphql/edge package and @typegraphql/node, but we're not ready to rewrite whole repo to support monorepo and multiple packages approach with proper build mechanism.

I think you should try to use some JS bundler to pack your source code with node_modules dependencies, just like we do for the browser:
https://parceljs.org/features/node-emulation/#polyfilling-%26-excluding-builtin-node-modules

This way, as the code is not actually called, replacing the imports with empty modules won't cause any runtime issues.

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community and removed Question ❔ Not future request, proposal or bug issue labels Oct 4, 2023
@ramiel
Copy link
Author

ramiel commented Oct 4, 2023

So, let me reply to each point

  • Use await import: I agree, this should not be done
  • going with subpath: yes, this is the only drawback I see, emitSchemaFile: options is lost. The alternative is not that bad though:
import {buildSchema} from 'type-graphql';
import {emitSchemaFile} from 'type-graphql-utils';

const schema = buildSchema();
emitSchemaFile({schema, filePath: 'schema.graphql'});
  • global is not really needed or a problem. Check how I changed the code in my package here

I know that building the project mocking node library looks a good solution, but in real world complex scenario is not. For example in my monorepo, with pnpm and tsup (esbuild) the resolution of packages lead to have the alias not working in deeped imported packages (where the imports are not aliased by design).

In any case, I fully understand if this is something the project doesn't want to do. In that case, feel free to close this issue, or maybe try to understand how much of your userbase is interested in this. Well, do what you feel right, of course :D

@MichalLytek
Copy link
Owner

global is not really needed or a problem. Check how I changed the code in my package here

This is a workaround. After releasing that "fix" there will be new issues opened that TypeGraphQL does not see the definitions from other packages, etc. 😕

@ramiel
Copy link
Author

ramiel commented Oct 4, 2023

What do you mean? The definition is the only untouched part

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 4, 2023

I mean the definitions - classes decorated with TypeGraphQL's decorators. Like someones publish Relay helpers for TypeGraphQL as an npm package - in some edge cases you might end in multiple version of type-graphql in your node_modules. And since const cachedStorage = new MetadataStorage(); is a local declaration, the metadata storages will be separated and the built schema won't be matching your expectation (missing args, types, etc.).

@ramiel
Copy link
Author

ramiel commented Oct 4, 2023

Oh, I see what you mean. The current solution in type-graphql is a workaround to actually be sure there's only one instance of typegraphql in node_modules. For example it's the same with the graphql package after all, where you cannot have two different versions installed (not even with the same version number), this is why it's usually set as a peer dependency.

Anyway, for the general discussion of this issue, I'd suggest to maybe close it. I understand this is not a direction this project wants to pursuit and there's nothing bad. I will try to come up with another package that follow yours. I also have to do the same for typegraphql-prisma. Maybe you can revisit this proposal or a similar one in the future.

Thanks for all the attention!

@MichalLytek
Copy link
Owner

Anyway, for the general discussion of this issue, I'd suggest to maybe close it. I understand this is not a direction this project wants to pursuit and there's nothing bad. I will try to come up with another package that follow yours. I also have to do the same for typegraphql-prisma. Maybe you can revisit this proposal or a similar one in the future.

Supporting all runtimes is the direction I want to follow with this project.
However, not everything is easily doable right now.

The issue with fs will be solved as soon as we switch to the monorepo and separate packages. Then the core could be run on any platform as it would be almost zero deps (only graphql-js).

The issue with global might be not relevant in today times.
I, and other users, had issues mostly because of npm link and other solutions where you have multiple node_modules.
However, now with monorepos, you can dedupe the type-graphql package and have only a single instance both in the app and in the libs. That's what graphql-js with the "another realm" error.
So this one could be fixed soon with some conditional check for existence of global and if not, then using local variable as storage mounting point.

@ramiel
Copy link
Author

ramiel commented Mar 6, 2024

This message makes me very happy. I'll also be very happy of helping if there's any need

@ceddybi
Copy link

ceddybi commented Oct 8, 2024

+1 for edge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants
@ramiel @Enalmada @ceddybi @MichalLytek @akhdefault and others