tatethurston / nextjs-routes

Type safe routing for Next.js
MIT License
566 stars 23 forks source link

Fix: 152 align documentation on nextjs project dir #153

Closed airtonix closed 1 year ago

airtonix commented 1 year ago

fixes #152

airtonix commented 1 year ago

So some unsolictated advice, do with it what you want.

Some suggestions:

tatethurston commented 1 year ago

So some unsolictated advice, do with it what you want.

  • cant install the repo for dev mode, so can't make commits, so had to use --no-verify

    • this is caused by the e2e directory has a package.json that wants to install a monorepo package (I assume the root package), problem is that the root package is called nextjs-routes-development and not nextjs-routes like the e2e package wants.

Some suggestions:

  package.json
  pnpm-workspace.yaml
  README.md
  pkgs/
    nextjs-routes/
      package.json
    nextjs-routes-e2e/
      package.json
  • drop prettier-package-json and switch to yarn4 with some of its linting tools, or if you cant switch to yarn 4 then use syncpack to lint/reformat package.json and keep it clean
  • use changesets to automate changelogs and releases

Thanks, I'll consider these. I haven't had many contributors, so I haven't prioritized documenting local development.

airtonix commented 1 year ago

Thanks @airtonix. A few things:

  1. The documentation changes reference cwd, but this code change renames to dir.

😓 😄 woops. it should be dir so it's not a breaking change.

  1. Renaming the configuration option from dir to cwd is a breaking change. I'm open to thinking on a more descriptive name.

👍🏻

Thanks, I'll consider these. I haven't had many contributors, so I haven't prioritized documenting local development.

I just realised that latest version of nextjs has some kind of typesafe routing behind a experimental feature flag: https://beta.nextjs.org/docs/configuring/typescript#statically-typed-links

tatethurston commented 1 year ago

I just realised that latest version of nextjs has some kind of typesafe routing behind a experimental feature flag: https://beta.nextjs.org/docs/configuring/typescript#statically-typed-links

Yes, I've held off on implementing support for the app directory because Next now supports that natively. If your application exclusively uses the app directory, I would suggest using that solution instead.

tatethurston commented 1 year ago

😓 😄 woops. it should be dir so it's not a breaking change.

It's actually the opposite -- it should be cwd so that it's non breaking.

tatethurston commented 1 year ago

I implemented your suggested change to use the cwd/dir option for the default location for type generation: https://github.com/tatethurston/nextjs-routes/pull/156

tatethurston commented 1 year ago

I'm inclined to solve this without any code changes and only update the README.md

tatethurston commented 1 year ago

Thanks!