dotindustries / bouncer

SaaS subscription seat manager
https://bouncer.dot.industries
GNU General Public License v3.0
3 stars 3 forks source link

[Zodios] upgrade #1

Closed ecyrbe closed 2 years ago

ecyrbe commented 2 years ago

Hello,

I'm happy to see an advanced project using zodios in public. If you have feedback don't hesitate to go on zodios discussion page. i'm pretty active. If you have a bug, it's the same.

This issue is just to tell you that Zodios had pretty bad Bugs that where higlighted thanks to the migration to react-query v4. If you inspect the generated bundle with your today nextjs app, you'll see some strange things with many polyfills.

The issue was fixed with new versions of zodios, just upgrade to the latests. 'v10.3.1' for core. Also there are some fixes on typesafety part, that should make your like easier.

whish you and your project all the success!

nadilas commented 2 years ago

Hello @ecyrbe, nice of you to chime in 🙏 Thanks for the hint, just upgraded to the latest versions.

There are currently three things, which cause somewhat of a headache:

ecyrbe commented 2 years ago

For the third point, it's actually normal. before zodios failed to tell your you forgot to pass parameters.

to fix, just pass the required parameters:

const { mutate } = userClientHooks.useMutation(
    "post",
    "/subscriptions/:subscriptionId/seats/:seatId/request",
    {
       subscriptionId: <your subscription id>,
       seatId: <your seat id>
    },
    {
      onSuccess: () => invalidate(),
    }
  );
ecyrbe commented 2 years ago

for your middleware, remember that zodios is just express with types, so this should work :

app.use( async (req, _, next) => {
   req.repo = createSqliteRepository({
     database: sqlite,
     onCreateConnection: async () => console.log("connected to database"),
   });

   return next(); // do not pass any argument to next, else it will be transformed to an error 500
});
nadilas commented 2 years ago

For the third point, it's actually normal. before zodios failed to tell your you forgot to pass parameters.

to fix, just pass the required parameters:

const { mutate } = userClientHooks.useMutation(
    "post",
    "/subscriptions/:subscriptionId/seats/:seatId/request",
    {
       subscriptionId: <your subscription id>,
       seatId: <your seat id>
    },
    {
      onSuccess: () => invalidate(),
    }
  );

Alright, that makes sense 👍

nadilas commented 2 years ago

for your middleware, remember that zodios is just express with types, so this should work :

app.use( async (req, _, next) => {
   req.repo = createSqliteRepository({
     database: sqlite,
     onCreateConnection: async () => console.log("connected to database"),
   });

   return next(); // do not pass any argument to next, else it will be transformed to an error 500
});

I'll try this right away. I was messing up no the not providing params to next. I was thinking in trpc contexts, that's why I wanted to outsmart typescript.

ecyrbe commented 2 years ago

For the first, there seems to be something wrong with how i packaged @zodios/express. In the mean time, you should revert to @zodios/express 1.0.5. I'll try to come with a patch as fast as i can.

nadilas commented 2 years ago

Thanks, reverted.

There's also a weird one after the update and adding another route: https://github.com/dotindustries/bouncer/blob/b24f8356fd8b32bec5032f4363efbdeb44ceb8f9/apps/api/pages/index.tsx#L24

api:dev: Error: No QueryClient set, use QueryClientProvider to set one
api:dev:     at useQueryClient (file:///Users/nadilas/workspace/dot-inc/bouncer/node_modules/.pnpm/@tanstack+react-query@4.13.0_biqbaboplfbrettd7655fr4n2y/node_modules/@tanstack/react-query/build/lib/QueryClientProvider.mjs:34:11)
ecyrbe commented 2 years ago

Can you remove the workaround in your nextjs.config.mjs ? i think it should work without it.

ecyrbe commented 2 years ago

it should look like this :

// import ntm from "next-transpile-modules";
import { createRequire } from "module";

const require = createRequire(import.meta.url);
// const withTM = ntm(["@dotinc/bouncer-core"]);

/**
 * Don't be scared of the generics here.
 * All they do is to give us autocompletion when using this.
 *
 * @template {import('next').NextConfig} T
 * @param {T} config - A generic parameter that flows through to the return type
 * @constraint {{import('next').NextConfig}}
 */
function defineNextConfig(config) {
  return config;
}

export default // withTM(
defineNextConfig({
  reactStrictMode: true,
  swcMinify: true,
});
nadilas commented 2 years ago

Yep, that solved it. 🎉 My reference was: https://github.com/ecyrbe/zodios-express/blob/main/examples/next/next.config.js

ecyrbe commented 2 years ago

Yes, i just updated the example to new version as suggested. This was a workaround, no more valid. sorry for that. Also i fixed packaging on @zodios/express 10.3.2.

I tested the packaging on the example. it's ok now. sorry for the inconvenience.

nadilas commented 2 years ago

No worries, no harm done. And thanks for being so engaged.

ecyrbe commented 2 years ago

also, one litle trick. if you want to enfocre a string on a path parameter you can, this way you'll be able to get rid of the custom validation you added:

{
    method: "post",
    alias: "publisherConfiguration",
    path: "/publisher/:publisherId/configuration",
    parameters: [
      {
        name: "publisherId",
        schema: z.string().regex(/<your regexp>/),
        type: "Path",
      },
      {
        name: "publisherConfiguration",
        schema: publisherConfiguration,
        type: "Body",
      },
    ],
    response: publisherConfiguration,
  },
nadilas commented 2 years ago

Thanks for the hint. I knew about the path param definition, but I actually found it nice that the path params were picked up without explicit definitions. I am somewhat annoyed by the string | number type, but I get why it's the default.