search

ethanniser / next-typesafe-url

Fully typesafe, JSON serializable, and zod validated URL search params, dynamic route params, and routing for NextJS.
https://next-typesafe-url.dev
MIT License
390 stars 17 forks source link

Make package flexible in monorepo #53

Closed nirtamir2 closed 1 year ago

nirtamir2 commented 1 year ago

This PR changes add 2 config options:

It also:

The code is inspired by https://github.com/Schniz/next-static-paths

nirtamir2 commented 1 year ago

Thanks for your quick response 💪

I created this PR to see what you think about the idea of setting the routes.d.ts in the userland code, committed to git (instead of the node_modules folder), and I see you are happy with it which is good for me. I want to use this library in one of my projects that structured differently and I hope the configs I've added will make it work there like next-static-paths (that does not intend to support app directory).

I'm not sure about the change you requested in the "types" folders. I'm ok with any change you want to make there (I specifically did not understand what you asked and which types should be private). BTW I'm not sure if dtsPath is descriptive enough. What do you think about changing it to output (like next-static-paths)?

I can create the changesets after you will be happy with the code and the API.

Thanks again for creating this library 👍

ethanniser commented 1 year ago

Thank your enthusiasm, feels great to know even one person is using the package :)

I took your feedback and made some changes:

I hope I understand it correctly. As I user of this library, I expect to import types from the root of "next-typesafe-url", not "next-typesafe-url/app". I don't want to import internal types as you mention. But I don't know what types are internal in this library - so it would be helpful if you tell me what you think - which type to export (maybe we need to have more examples).

Original my intention was to keep all of the types/functions only ever used in 'app mode' in /app and vice verse with pages, with shared stuff in index. (ie the Infer...PropsType would live in /app, while AppRouter is in /index)

In the long run, I don't see why would we use the app/pages flags at all. These are just implementation details. As next.js does not allow us to declare a page lives in the pages directory and have representation in the app directory - so I think that concept should not exist at all.

I don't want to import app/pages specific types if there is a type that handle both cases. Which types should I export?

You convinced me on the index type exports, but I still think having next-typesafe-url/app and next-typesafe-url/pages makes sense because for useSearchParams and useRouteParams because I want the external api to be the same between each 'version': same function name, inputs and outputs.

However, due to breaking changes on how route and search params are accessed between pages and app, the internal implementation has to be different.

So our options are:

  1. Pass ‘pages’ or ‘app’ as a function parameter’ const params = useSearchParams(Route.searchParams, ‘app’);

verbose, and doesn't make a lot of sense to be able to change it on every call given you only ever use one version in a given project

  1. Name the functions different names and export them from index: import { useAppSearchParams, usePagesSearchParams } from ‘next-typesafe-url';

I just think this one looks uglier than 3

  1. Same function name from different directory (what we are doing right now) import { useSearchParams } from ‘next-typesafe-url/app';

clean name, where you import it from changes what 'version' of the function you use

I think 3 is the best, but I am open to hearing your thoughts on this

thanks again

ethanniser commented 1 year ago

making some cli changes rn

ethanniser commented 1 year ago

made some really good cli changes the source of the original need for the 'dont use dev mode' was because of my again admittedly hacky previous generation technique, basically the cli was trying to modify the files in node_modules, but in the development repo those files where actually in ../../../packages, which caused even more hacky solutions

now that we are generated in a set location (thanks to your contribution) which makes things a million times easier

heres the api I ended up with:

Usage:
$ npx next-typesafe-url (...options)

Options:
--watch, -w Watch for routes changes
--pages, -p Pages directory
--app, -a App directory
--srcPath, The path to your src directory relative to the cwd the cli is run from. DEFAULT: "./src"
--outputPath, The path of the generated .d.ts file relative to the cwd the cli is run from. DEFAULT: "./generated/routes.d.ts"

so now in the dev monorepo everything actually just works with no additional flags, just next-typesafe-url -a/p

and changing input or output is as simple as a relative path from where the cli is run from

hopefully you like these changes too, let me know what you think

also realized looking through the docs portion of the repo astro does some code gen too, and stores a .astro/types.d.ts file

I am kind of leaning towards .next-typesafe-url/types.d.ts or .next-typesafe-url/routes.d.ts by default to keep the file clearly in its own folder, as generated could be used by other libraries. And if the user wants to change that they of course can.

ethanniser commented 1 year ago

was also just reading through the astro docs and they recommend git ignoring the generated files

Considering the generation step is deterministic from the rest of your code which is git tracked, Just out of curiosity what is your reason for wanting the generated file git tracked is?

nirtamir2 commented 1 year ago

Hi @ethanniser Amazing work!

Remove split in CLI for apps/pages

I think that we don't need to split between app and pages at all. Why do we need the -a / -p flags? Some apps have both pages routes and also app router routes. But if the implementation is hard - we can keep it that way for now.

In next.js you can use 

app/a/page.tsx
pages/b.tsx

But you get an error having 

app/a/page.tsx
pages/a.tsx

Which is a good thing - because routes should not have conflicts. So we can do the work for -a & -p flags at the same time. Having said that, I use this library mainly for the app directory.

import { useAppSearchParams, usePagesSearchParams } from ‘next-typesafe-url';

For the suggestions you made - I think I prefer (2): import { useAppSearchParams, usePagesSearchParams } from ‘next-typesafe-url'; because it's more verbose and can be easily imported by the IDE (no conflicts when importing leading to better experience). Yes - I know that next.js did import from next/navigation and next/routeand I don't like it at all. I even created an ESLint rule in my work's codebase to use the app directory import only:

    {"@typescript-eslint/no-restricted-imports": [
      "error",
      {
        patterns: [
         {
             group: ["next/route"],
            message: "import from next/navigation because we use next 13 app directory",
         },
        ],
      },
    ]}

because users may import it from the wrong place, and I don't want to have bugs/errors in my codebase.

Naming of values in CLI

outputPath is a great name 💪 For .next-typesafe-url/types.d.ts - I don't care as much as the user allows to change it itself (which is part of this PR). If you ask me - next-typesafe-url.d.ts works for me (if it works), but also what you've done seems good. This is the default value so the user can change it for wherever he wants.

Committing to git

For the

recommend git ignoring the generated files

I believe this is not related. Like .next directory is in .gitignore. Types are not ignored like in next-static-paths. The types depend on the running of the generate command, so they can be out of sync - so committing them to git is a good thing in my opinion. If you want an example for something that generates it directly to node_modules and is not committed into git - see Prisma which requires a command to be run in postinstall hook (which is unsafe and hard in the CI env etc). https://www.prisma.io/docs/concepts/components/prisma-client/working-with-prismaclient/generating-prisma-client#generating-prisma-client-in-the-postinstall-hook-of-prismaclient Better to avoid such things, but I can understand why this works for Prisma - the amount of types is bigger in an order of magnitude. So it's better to keep it in git. I use next-static-paths and even now when it does not support the app directory - I use it and manually update static-paths.d.ts to new routes (until this repo will satisfy my concerns 😉).

Again, thanks for the help and care :)

ethanniser commented 1 year ago

Committing to git

The git ignore thing at the end of a day is unenforceable by a library, and is up to the developer however they want to use it, I was just curious. I do understand what you mean though and your reasons for choosing to do so make sense.

Remove split in CLI for apps/pages

You are very right to point out there is actually no need for the -p -a flags, I will work on getting rid of those tonight and merging the previous functionality.

Naming

As for the naming I do understand your points, but I'll admit I'm still torn. I will think about it some more and ask some other people what they think.

Also just fyi im 90% sure next-typesafe-url will be featured in this week's 'This Week In React' newsletter, which I'm pretty sure will come out on Wednesday so I'm really pushing to try and merge this before then so all of the people who see the package see the best version we have.

At this point with the amount of changes I think this will be a 'Major' release which is actually very exciting because we greatly improved the package!

Thanks again @nirtamir2

ethanniser commented 1 year ago

also documenting here so I remember when writing the docs but if you set the outputDir to a directory that starts with . you must explicitly include it in tsconfig

merging app + pages is next

ethanniser commented 1 year ago 
// index.ts
export { useSearchParams as usePagesSearchParams } from "./pages";
export { useSearchParams as useAppSearchParams } from "./app";
export { useRouteParams as useAppRouteParams } from "./app";
export { useRouteParams as usePagesRouteParams } from "./pages";

I think this is a good compromise, and either way of accessing points to the exact same function

nirtamir2 commented 1 year ago

I believe this is not a good compromise. Personally, I'm not going to import the way you show in this example. I'd instead import {useSearchParams} from next-typesafe-url/app or from next-typesafe-url/page without changing the import name. But if you want to follow the next.js convention with next/navigation / next/router import - you can do it. This is legit.

The drawback is the tooling of the IDE which is inconvenient to import the right import image

But I'm ok with that as long as the runtime usage of it will produce an error if used wrong.

ethanniser commented 1 year ago

Personally, I'm not going to import the way you show in this example.

Im confused as this provides an identical experience to what you were pushing for earlier

For the suggestions you made - I think I prefer (2): import { useAppSearchParams, usePagesSearchParams } from ‘next-typesafe-url'; because it's more verbose and can be easily imported by the IDE

image image

Maybe I dont understand what you were asking for?

nirtamir2 commented 1 year ago

I prefer all imports will be from the main package and methods will be unique import {useAppSearchParams} from "next-typesafe-url.

If you don't want to do it (because you want to use useSearchParams which is a better name), I would use it as you suggest by import {useSearchParams} from "next-typesafe-url/app" but I won't import it like import {useSearchParams as useAppSearchParams} from "next-typesafe-url/app".

Both are legit ways to do it. The problem with the first approach is migration when the page's router will be deprecated, and the searhablility of the useAppSearchParams function (the name is different so the user may not remember it exists in the first place and won't use it). The problem with the second approach is choosing the right import/auto import problem.

Just choose one. If you prefer the second approach - go for it. It sounds good too 💪

nirtamir2 commented 1 year ago

BTW Why would you have 2 versions of useSearchParams? I notice we only have it in next/navigation (we don't have such a thing on pages).

UPDATE: Oh - I now see this creates a useless double rerenr (by calling setData, setError). I think it's better to handle it differently with notFound() or throw an error with zodSchema.parse instead.

I think it will be even better to remove this function from the library at all. You just pass the zod object and parse it inside - and it have more value doing so in the userland instead of this library. Same for useRouteParams I think it's better to remove this API at all and just focus on the $path function.

ethanniser commented 1 year ago

naming

but I won't import it like import {useSearchParams as useAppSearchParams} from "next-typesafe-url/app".

maybe you didnt understand what I was showing, youd never need to do this in your own code because its already done internally

image this leads to image

Just choose one

man, naming is difficult, I'm gonna push it off a bit longer while I work on the cli

the purpose of the package

I think it's better to remove this API at all and just focus on the $path function.

I think were just at different points with the ideal use case here. My original whole reason for making the package was having runtime validation. The typesafe routing has existed in the past, next-static-paths and pathpida (this might be what your looking for- fully supports app dir, which is totally cool btw not everyone wants the extra validation stuff). What next-typesafe-url does thats unique is it makes the consumption of search/route params typesafe (by default everything is string | string[] | undefined). Thats the whole reason for zod, the hooks, everything really.

Now you actually don't have to use any of that. You can just use $path for routing, and then get your search params the default way from next and do whatever validation you want. But next-typesafe-url provides functions that make that seamless, as well as some custom serialization/deserialization for supporting JSON.

I will not be removing that functionality

image

nirtamir2 commented 1 year ago

Yea, you're right. We have different visions. Notice that at the time of writing this is the only library I know that supports the app router - which is valuable for me. So I think we can close the debate here because we have different visions (I won't use the useSearchParams etc). Still, thank you again for creating this library 💯 Still waiting for the changes to be merged, but no pressure 👍

ethanniser commented 1 year ago

Sounds, good. again definately check out pathpida:

Type safety. Automatically generate type definition files for manipulating internal links in Next.js/Nuxt.js. Zero configuration. No configuration required can be used immediately after installation. Zero runtime. Lightweight because runtime code is not included in the bundle. Support for static files. Static files in public/ are also supported, so static assets can be safely referenced. Support for appDir of Next.js 13 Layout.

I'm also leaving this as a note to myself to add a reference to pathpida in the docs, not everyone wants runtime stuff so they are a great option

If you still want to use next-typesafe-url I am going to work hard to update the docs and get this merged tonight hopefully

Even thought we have different use cases I truly appreciate your perspective and challenging of my ideas. It's very easy when working on something all by yourself to not question certain decisions that have big impacts. This PR has seriously improved the package, so sincerely thank you.

nirtamir2 commented 1 year ago

I prefer your / next-static-paths styles for using the path with a static string that can be easily analyzed and searched as a string with CMD+SHIFT+F. So I will use your library for that (I hope this won't matter because of three-shaking). This is my replacement for next-static-paths that support the app directory. I could use the experimental next typeRoutes (and I open my RFC to nextjs about it https://github.com/vercel/next.js/discussions/50118) but this not giving me static analysis. BTW - you can add a feature for future versions that expose functions like $path with a return type of the new experimenta.typeRoutes Route (import type { Route } from 'next';) type instead of string for easy integration.

ethanniser commented 1 year ago

Making the decision to go with useSearchParams from /pages or /app.

Unless theres anything else you can think of @nirtamir2 I'm going to start working on updating the docs.

If you able to put together a full list of changes for the changset that would be amazing, but if not I can do that too no big deal just let me know

Thanks again

ethanniser commented 1 year ago

everything looks good to me Im working on the docs right now and will hopefully merge to main tonight thank you so so much