MichalLytek / type-graphql

Create GraphQL schema and resolvers with TypeScript, using classes and decorators!
https://typegraphql.com
MIT License
8.03k stars 676 forks source link

Improved browser shim #673

Closed desmap closed 4 months ago

desmap commented 4 years ago

The UX of current browser shim feature is currently hardly usable and might be significantly improved.

_tl/dr the proposed solution: type-graphql just knows if it shall deliver the real module or the shim via an env var which is set in the Docker/k8s orchestration per service such as TYPE_GRAPHQL_SHIM: 1_

A short wrap-up about the current UX: The shim is used by importing typegraphl/dist/browser-shim.js instead of type-graphl. So, you need to be aware that depending on your context/package you have to import either the real module or the shim. And it must be always clear always both (1) tsc at build time and (2) node at runtime must be aware of this. The user has to take care that both in 1 and 2 this "redirection" is really happening.

The Problem: While former is possible in many ways, most of the solutions are hacky and create complications and eventually just don't work together in a monorepo whose packages in production are reflected as isolated Docker containers/k8s services. I don't want to list all the problems but just give a hint: We face so many issues, setting up monorepos is already no trivial thing (just think about file organization or TS' quite restricted + complicated project references features which shakes up your whole dir structure, TS paths feature can't be used always), then we have three competing package managers, one can't do monorepos, another one which future is unclear and doesn't support ESM (yarn) and another which does not have an ecosystem yet (pnpm) and finally we need to think, implement and remember n m solutions for this redirection (n = packages, m = 2 = a solution at build and at runtime). These n m implementations need to be also interoperable. Good luck or tbh, it's not possible.

The Solution: type-graphql just knows if it shall deliver the real module or the shim via env var which is set in the Docker/k8s orchestration per service such as TYPE_GRAPHQL_SHIM: 1, then we wouldn't need any hacky redirection which go deep in the build system.

Maybe there are better solutions but I know that current one is not really working. The build process/system was taking 99% of my time the last weeks and only because I wanted to share some types and class validators. I don't think that code generation —I could just cpx the files on change into some other packages—shouldn't be the solution for proper code sharing.

MichalLytek commented 4 years ago

The Solution: type-graphql just knows if it shall deliver the real module or the shim via env var

How it gonna prevent into bundling the whole graphql-js library then?

We face so many issues, setting up monorepos is already no trivial thing

So maybe just adapt your own simple solution? Create a common/typegraphql.ts file where you do the logic behind the env variable and just do import {} from "@common/typegraphql" instead of import {} from "type-graphql" in your code?

desmap commented 4 years ago

How it gonna prevent into bundling the whole graphql-js library then?

dynamic imports?

Create a common/typegraphql.ts file where you do the logic behind the env variable

This is actually a good idea. I could give it a try with pnpm which supports both monorepos and I believe also ESM. Before I start, does this make sense or should I head in a different direction?

MichalLytek commented 4 years ago

dynamic imports?

How then you gonna be able to import something like decorator from such entrypoint?

desmap commented 4 years ago

No I just realize that dynamic import need to package the whole dir...

ok, then there might be no solution??

desmap commented 4 years ago

How then you gonna be able to import something like decorator from such entrypoint?

What do you mean with this?

MichalLytek commented 4 years ago

What do you mean with this?

Read about static export in es modules and dynamic imports.

ok, then there might be no solution??

You need a plugin for your bundle system that gonna do the thing you want to do - for some packages replace with the shim (client), for some of them leave the original module (api).

You can't do that in the runtime based on the env variable value. The goal of the shim is to only bundle 1KB not 1MB of code when you import shared code in frontend app.

desmap commented 4 years ago

You can't do that in the runtime based on the env variable value.

Ok, makes sense. The we can close this issue because my proposed solution is actually crap. 😳

However, this build process is really killing me and maybe I just cpx those files into consuming packages. Or how do you do this?

MichalLytek commented 4 years ago

I had a simple common-front-back monorepo and in front I did the webpack bundle trick for CRA only.

Why you can't configure the shim per monorepo project? What are your issues with that? Is that still the Next.js thing?

desmap commented 4 years ago

Don't want to go to deep:

desmap commented 4 years ago

When I remember it right, I couldn't alias the shim with TS' references features at all (depending on the consuming package), so yeah

desmap commented 4 years ago

again re my initial idea with dynamic imports: eg Parcel does have zero configuration code splitting using dynamic import() statements. So would code splitting not help at runtime, so the full lib should not be loaded but just the shim?

And I think Next has code splitting based on dynamic imports too.

MichalLytek commented 4 years ago

Go ahead, figure it out, try different ideas and go back with the clear and simple solution we can put in the readme or in the code.

Your use case is super rare and super specific, I don't have time to deliberate more on that.

desmap commented 4 years ago

Go ahead, figure it out

https://github.com/desmap/type-graphql-fix-shim-example

clear and simple solution

We even don't need dynamic imports, just a small mod of your browser-shim.ts is required => https://github.com/desmap/type-graphql-fix-shim-example/blob/master/fix-tgql/src/index.ts

So this would even run without ESM and with full tree-shaking.

Options now:

Your use case is super rare and super specific

Running Next in a container orchestration is super rare and super specific??

Re Next https://www.npmtrends.com/next-vs-type-graphql re being containerized: 1B downloads of nodes Docker image.

I don't have time to deliberate more on that.

We all don't have time, time is money, no need to tell me this. In this case, I think this might benefit this lib and your users and eventually the reach/success of type-graphql more than it would help me. But it's your call.

MichalLytek commented 4 years ago

Running Next in a container orchestration is super rare and super specific??

You are the first one after 2 years that ask for doing that.

We all don't have time, time is money, no need to tell me this. In this case, I think this might benefit this lib and your users and eventually the reach/success of type-graphql more than it would help me. But it's your call.

I have to choose - work on a shim for Next and monorepo or work on the dataloader integration or type transform utils that have much more thumbs up than your request.

desmap commented 4 years ago

Ok busy man, here my PR https://github.com/MichalLytek/type-graphql/pull/674

desmap commented 4 years ago

a nice side effect with that PR: we can replace the current shim documentation to four words: enable shim with TYPE_GRAPHQL_SHIM=1

Isn't this a great UX?! :)

MichalLytek commented 4 months ago

The goal of the shim is to prevent bundling 1MB payload of type-graphql with graphql-js and other. This has to be done on bundle time, not in runtime. Having env variable and if statements rely on the fact that tree-shaking works without any issue and bundler/compiler will remove the real code, loading only the shim. I don't find it useful embedding this pattern directly into the core package. If standard shim approach via tsconfig or webpack does not work well for someone, it's easily possible to expose internal re-export file that will read env variable and return shim or real type-graphql code.