type-graphql icon indicating copy to clipboard operation
type-graphql copied to clipboard

Improved browser shim

Open desmap opened this issue 5 years ago • 16 comments

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.

desmap avatar Jul 27 '20 08:07 desmap

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?

MichalLytek avatar Jul 27 '20 08:07 MichalLytek

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?

desmap avatar Jul 27 '20 08:07 desmap

dynamic imports?

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

MichalLytek avatar Jul 27 '20 08:07 MichalLytek

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

ok, then there might be no solution??

desmap avatar Jul 27 '20 08:07 desmap

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

What do you mean with this?

desmap avatar Jul 27 '20 08:07 desmap

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.

MichalLytek avatar Jul 27 '20 08:07 MichalLytek

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?

desmap avatar Jul 27 '20 08:07 desmap

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?

MichalLytek avatar Jul 27 '20 09:07 MichalLytek

Don't want to go to deep:

  • For Next.js I need also to declare at runtime because Next runs it own backend server (which I don't use as my real API server btw), so then I need tsconfig-paths for the runtime redirection
  • tsconfig-paths doesn not work with ESM, the maintainer suggests rather to use TS' references feature
  • TS references are quite intrusive, your dir structure shouldn't contain src anymore and there is a lot of other stuff
  • However and even with TS references I need still to care about runtime (for Next) and TS references opens another can of worms

desmap avatar Jul 27 '20 09:07 desmap

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 avatar Jul 27 '20 09:07 desmap

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.

desmap avatar Jul 27 '20 09:07 desmap

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.

MichalLytek avatar Jul 27 '20 09:07 MichalLytek

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:

  • everybody can do this themselves but then they have to lock type-graphql and need to check if you changed/extended the API and they need some smart package manager like pnpm which can do monorepos without pain (npm and yarn are out here)
  • I could publish a ded peer dep like fix-type-graphql but this would feel odd
  • we integrate this in the main repo as second option to active the shim

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.

desmap avatar Jul 28 '20 08:07 desmap

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.

MichalLytek avatar Jul 28 '20 09:07 MichalLytek

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

desmap avatar Jul 28 '20 12:07 desmap

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?! :)

desmap avatar Jul 28 '20 12:07 desmap