Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[legacy-framework] Add autogenerated Routes manifest for use with <Link> components #2042

Merged
merged 43 commits into from
Apr 10, 2021

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Mar 3, 2021

Closes: blitz-js/legacy-framework#513

What are the changes and their implications?

Give users a Route manifest as described in blitz-js/legacy-framework#513.

ToDo:

  • Generate manifest
  • Decide on where the manifest should be generated into
  • Do TypeScript shenanigans to allow importing from blitz

Checklist

  • Changes covered by tests (tests added if needed)
  • PR submitted to blitzjs.com for any user facing changes

Skn0tt added 3 commits March 3, 2021 10:00
This stage creates a manifest about all routes.
For the Auth example, this is what it looks like:

```ts
export default {
  Home: "/",
  Test: "/ssr",
  ForgotPasswordPage: "/forgot-password",
  LoginPage: "/login",
  ResetPasswordPage: "/reset-password",
  SignupPage: "/signup",
  ShowProjectPage: ({ projectId }: { projectId: string }) => `/projects/${projectId}`,
  ProjectsPage: "/projects",
  NewProjectPage: "/projects/new",
  EditProjectPage: ({ projectId }: { projectId: string }) => `/projects/${projectId}/edit`
}
```

This can be imported from user code to use as described in #2023.
@Skn0tt Skn0tt changed the title 2023-route-manifest #2023: Give users a Route Manifest Mar 3, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2021

Size Change: +4.57 kB (+2%)

Total Size: 239 kB

Filename Size Change
packages/file-pipeline/dist/blitzjs-file-pipeline.cjs.dev.js 7.14 kB +256 B (+4%)
packages/file-pipeline/dist/blitzjs-file-pipeline.cjs.prod.js 7.14 kB +256 B (+4%)
packages/file-pipeline/dist/blitzjs-file-pipeline.esm.js 6.86 kB +255 B (+4%)
packages/server/dist/blitzjs-server.cjs.dev.js 13.9 kB +1.26 kB (+10%) ⚠️
packages/server/dist/blitzjs-server.cjs.prod.js 13.9 kB +1.26 kB (+10%) ⚠️
packages/server/dist/blitzjs-server.esm.js 13.6 kB +1.27 kB (+10%) ⚠️
ℹ️ View Unchanged
Filename Size Change
packages/babel-preset/dist/blitzjs-babel-preset.cjs.dev.js 1.69 kB 0 B
packages/babel-preset/dist/blitzjs-babel-preset.cjs.js 150 B 0 B
packages/babel-preset/dist/blitzjs-babel-preset.cjs.prod.js 1.69 kB 0 B
packages/babel-preset/dist/blitzjs-babel-preset.esm.js 1.63 kB 0 B
packages/blitz/cli/dist/blitz-cli.cjs.dev.js 1.47 kB 0 B
packages/blitz/cli/dist/blitz-cli.cjs.js 142 B 0 B
packages/blitz/cli/dist/blitz-cli.cjs.prod.js 1.47 kB 0 B
packages/blitz/cli/dist/blitz-cli.esm.js 1.37 kB 0 B
packages/blitz/custom-server/dist/blitz-custom-server.cjs.dev.js 288 B 0 B
packages/blitz/custom-server/dist/blitz-custom-server.cjs.js 149 B 0 B
packages/blitz/custom-server/dist/blitz-custom-server.cjs.prod.js 288 B 0 B
packages/blitz/custom-server/dist/blitz-custom-server.esm.js 123 B 0 B
packages/blitz/dist/blitz.cjs.dev.js 355 B 0 B
packages/blitz/dist/blitz.cjs.js 139 B 0 B
packages/blitz/dist/blitz.cjs.prod.js 355 B 0 B
packages/blitz/dist/blitz.esm.js 102 B 0 B
packages/config/dist/blitzjs-config.cjs.dev.js 1.16 kB 0 B
packages/config/dist/blitzjs-config.cjs.js 146 B 0 B
packages/config/dist/blitzjs-config.cjs.prod.js 1.16 kB 0 B
packages/config/dist/blitzjs-config.esm.js 1.03 kB 0 B
packages/core/config/dist/blitzjs-core-config.cjs.dev.js 260 B 0 B
packages/core/config/dist/blitzjs-core-config.cjs.js 150 B 0 B
packages/core/config/dist/blitzjs-core-config.cjs.prod.js 260 B 0 B
packages/core/config/dist/blitzjs-core-config.esm.js 73 B 0 B
packages/core/dist/blitz-data-080e83b6.cjs.dev.js 1 kB 0 B
packages/core/dist/blitz-data-11f14b56.cjs.prod.js 1 kB 0 B
packages/core/dist/blitz-data-d1f53a00.esm.js 910 B 0 B
packages/core/dist/blitzjs-core.cjs.dev.js 8.69 kB 0 B
packages/core/dist/blitzjs-core.cjs.js 144 B 0 B
packages/core/dist/blitzjs-core.cjs.prod.js 8.42 kB 0 B
packages/core/dist/blitzjs-core.esm.js 8.39 kB 0 B
packages/core/dist/constants-010bc79f.cjs.dev.js 2.92 kB 0 B
packages/core/dist/constants-fbc3a7f6.esm.js 2.82 kB 0 B
packages/core/dist/constants-fcf80a42.cjs.prod.js 2.92 kB 0 B
packages/core/dist/extends-1b905a27.esm.js 241 B 0 B
packages/core/dist/extends-93eedbb0.cjs.dev.js 250 B 0 B
packages/core/dist/extends-f26277ce.cjs.prod.js 250 B 0 B
packages/core/document/dist/blitzjs-core-document.cjs.dev.js 448 B 0 B
packages/core/document/dist/blitzjs-core-document.cjs.js 151 B 0 B
packages/core/document/dist/blitzjs-core-document.cjs.prod.js 450 B 0 B
packages/core/document/dist/blitzjs-core-document.esm.js 272 B 0 B
packages/core/dynamic/dist/blitzjs-core-dynamic.cjs.dev.js 263 B 0 B
packages/core/dynamic/dist/blitzjs-core-dynamic.cjs.js 151 B 0 B
packages/core/dynamic/dist/blitzjs-core-dynamic.cjs.prod.js 263 B 0 B
packages/core/dynamic/dist/blitzjs-core-dynamic.esm.js 73 B 0 B
packages/core/head/dist/blitzjs-core-head.cjs.dev.js 245 B 0 B
packages/core/head/dist/blitzjs-core-head.cjs.js 148 B 0 B
packages/core/head/dist/blitzjs-core-head.cjs.prod.js 245 B 0 B
packages/core/head/dist/blitzjs-core-head.esm.js 64 B 0 B
packages/core/image/dist/blitzjs-core-image.cjs.dev.js 249 B 0 B
packages/core/image/dist/blitzjs-core-image.cjs.js 149 B 0 B
packages/core/image/dist/blitzjs-core-image.cjs.prod.js 249 B 0 B
packages/core/image/dist/blitzjs-core-image.esm.js 65 B 0 B
packages/core/server/dist/blitzjs-core-server.cjs.dev.js 13 kB 0 B
packages/core/server/dist/blitzjs-core-server.cjs.js 148 B 0 B
packages/core/server/dist/blitzjs-core-server.cjs.prod.js 13 kB 0 B
packages/core/server/dist/blitzjs-core-server.esm.js 12.9 kB 0 B
packages/core/with-blitz/dist/blitzjs-core-with-blitz.cjs.dev.js 1.29 kB 0 B
packages/core/with-blitz/dist/blitzjs-core-with-blitz.cjs.js 151 B 0 B
packages/core/with-blitz/dist/blitzjs-core-with-blitz.cjs.prod.js 1.29 kB 0 B
packages/core/with-blitz/dist/blitzjs-core-with-blitz.esm.js 1.15 kB 0 B
packages/display/dist/blitzjs-display.cjs.dev.js 2 kB 0 B
packages/display/dist/blitzjs-display.cjs.js 147 B 0 B
packages/display/dist/blitzjs-display.cjs.prod.js 1.95 kB 0 B
packages/display/dist/blitzjs-display.esm.js 1.86 kB 0 B
packages/file-pipeline/dist/blitzjs-file-pipeline.cjs.js 150 B 0 B
packages/generator/dist/blitzjs-generator.cjs.dev.js 14.2 kB 0 B
packages/generator/dist/blitzjs-generator.cjs.js 148 B 0 B
packages/generator/dist/blitzjs-generator.cjs.prod.js 14.2 kB 0 B
packages/generator/dist/blitzjs-generator.esm.js 13.8 kB 0 B
packages/generator/dist/templates/app/babel.config.js 78 B 0 B
packages/generator/dist/templates/app/blitz.config.js 310 B 0 B
packages/generator/dist/templates/app/jest.config.js 60 B 0 B
packages/installer/dist/blitzjs-installer.cjs.dev.js 7.52 kB 0 B
packages/installer/dist/blitzjs-installer.cjs.js 148 B 0 B
packages/installer/dist/blitzjs-installer.cjs.prod.js 7.52 kB 0 B
packages/installer/dist/blitzjs-installer.esm.js 7.3 kB 0 B
packages/repl/dist/blitzjs-repl.cjs.dev.js 1.75 kB 0 B
packages/repl/dist/blitzjs-repl.cjs.js 144 B 0 B
packages/repl/dist/blitzjs-repl.cjs.prod.js 1.75 kB 0 B
packages/repl/dist/blitzjs-repl.esm.js 1.6 kB 0 B
packages/server/dist/blitzjs-server.cjs.js 145 B 0 B

compressed-size-action

@Skn0tt
Copy link
Member Author

Skn0tt commented Mar 9, 2021

Alright, so now the route manifest is generated into node_modules/.blitz/route-manifest.ts. For the auth example, it'd look like this:

export default {
  Home: "/",
  Test: "/ssr",
  ForgotPasswordPage: "/forgot-password",
  LoginPage: "/login",
  ResetPasswordPage: "/reset-password",
  SignupPage: "/signup",
  ShowProjectPage: ({ projectId }: { projectId: string }) => `/projects/${projectId}`,
  ProjectsPage: "/projects",
  NewProjectPage: "/projects/new",
  EditProjectPage: ({ projectId }: { projectId: string }) => `/projects/${projectId}/edit`
}

For our last TODO "Do TypeScript shenanigans to allow importing from blitz", I need some inspiration. What's the best way of making the manifest accessible to devs?

@flybayer
Copy link
Member

flybayer commented Mar 9, 2021

For our last TODO "Do TypeScript shenanigans to allow importing from blitz", I need some inspiration. What's the best way of making the manifest accessible to devs?

Follow same approach as prisma, which I expect is adding this to @blitzjs/core index file:

export {default as Routes} from '.blitz/route-manifest.ts'

@Skn0tt
Copy link
Member Author

Skn0tt commented Mar 13, 2021

Adding export {default as Routes} from ".blitz/route-manifest" results in Error: Cannot find module '.blitz/route-manifest' on the first run b/c webpack tries to bundle the file before it is emitted. For blitz dev, this can be circumvented by emitting first - but for any other source code analysis, that error will be a bummer.
Shipping stub files is the way to go, I think. We can give them contents that give meaningful hints about "this is a stub!".

For housing the stubs, we could create a new package called @blitz/generated.
But: It would look like any other package, and in case of Prisma's .prisma style code gen ever becoming more common, we wouldn't profit from any optimisations in that regard (e.g. bundlers may decide to opt dev-generated deps out of caching optimisations).

Another solution would be to add a post-install script to blitz that creates stubs in .blitz.

Prisma has a proxy package @prisma/client that forwards to .prisma I think.

What do y'all think about this? It's not a decision that would require a breaking change in the future, so we don't need to ponder that long about it ^^

@Skn0tt
Copy link
Member Author

Skn0tt commented Mar 15, 2021

I've decided to resemble Prisma: We're still generating into node_modules/.blitz/route-manifest.js/d.ts, but now generate it during postinstall.

There's two problems that need to be solved, though:

  • I couldn't get export {default as Routes} from ".blitz/route-manifest" to work. It always complains about not finding the dependency, and I think it's caused by our monorepo structure. @flybayer maybe you could give it a try?
  • I added the command dx-generate, which is used for updating the manifest e.g. before unit tests in CI. I originally wanted to name it generate (like Prisma), but we already have that command. dx-generate is a bad name. How could we call it?

@flybayer
Copy link
Member

Yeah, I think do exactly what Prisma does unless we have a good reason not to. Also I would copy their code exactly as much as possible, because they have fixed a lot of things like postinstall not working in monorepos etc.

What exactly is the error with export {default as Routes} from ".blitz/route-manifest"? You might try with monorepo yarn build. There could be some slight issues we need to workaround with monorepo yarn dev because of the preconstruct no-build setup.

On the command naming:

My feeling is that we should use blitz generate if possible.

Here's some ideas

blitz init
blitz codegen
blitz bootstrap
blitz generate sidecar
blitz generate sdk // not good, prolly want for mobile app sdk
blitz generate init
blitz generate foundation
blitz generate lib

I think I quite like blitz generate lib. It's short and it clearly denotes some type of library files.

@flybayer
Copy link
Member

I suspect the .blitz module not found error may be coming from preconstruct as a warning that it's not a devDep or peerDep. If that's the case, then update our preconstruct patch to include .blitz. We already use patch-package to patch it to hide some of those errors for other libraries.

@flybayer
Copy link
Member

flybayer commented Apr 5, 2021

We also need to add docs for this.

And update the why-blitz page to include this as a key feature addition to next.js.

@Skn0tt
Copy link
Member Author

Skn0tt commented Apr 6, 2021

Some thoughts that occurred to me while writing the docs:

  1. What about query parameters like /?counter=10? We could allow a passthrough. Routes.Home({ counter: 10 }). That'd require us to make all routes a function, but I don't think that's a problem.
  2. At the moment, Routes.Product({ pid: 123 }) returns /products/123. Would it be better to return { pathname: "/products/[pid]", query: { pid: 123 } } instead? That could make the generated code more legible, especially with respect to 1.
  3. Is Pages a better naming than Routes? Route could also refer to API Route, Page is more specific.

@flybayer
Copy link
Member

flybayer commented Apr 6, 2021

  1. What about query parameters like /?counter=10? We could allow a passthrough. Routes.Home({ counter: 10 }). That'd require us to make all routes a function, but I don't think that's a problem.

Great point. Probably we should do this. And type the function so that if it's a dynamic route, the route param is required but also any key/value can be passed.

  1. At the moment, Routes.Product({ pid: 123 }) returns /products/123. Would it be better to return { pathname: "/products/[pid]", query: { pid: 123 } } instead? That could make the generated code more legible, especially with respect to 1.

This also sounds like a good idea.

  1. Is Pages a better naming than Routes? Route could also refer to API Route, Page is more specific.

I'm thinking we should stick with Routes so it leaves the door open for other things besides strictly pages.


Also, are we generating the manifest on blitz dev and blitz build? I feel like we should so it's always up to date. Pages change much more frequently than the prisma schema.

P.s. you have a merge conflict

@Skn0tt
Copy link
Member Author

Skn0tt commented Apr 7, 2021

Also, are we generating the manifest on blitz dev and blitz build? I feel like we should so it's always up to date. Pages change much more frequently than the prisma schema.

Yes, we already do. The generation mechanism reads from the normal pipeline, but instead of writing back into the pipeline it writes directly to .blitz.

@Skn0tt
Copy link
Member Author

Skn0tt commented Apr 7, 2021

**ERROR** Failed to apply patch for package @preconstruct/cli at path
    
      node_modules/@preconstruct/cli
  
    This error was caused because patch-package cannot apply the following patch file:
  
      patches/@preconstruct+cli+2.0.5.patch

🤔

I can't fix this locally (tried running npx patch-package @preconstruct/cli, but that didn't do anything to the patch file).
It worked before d5caf87, though. Could you take a look at it?

Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet!! Almost there. I didn't comment on all places needing changed below. Just once per file.

Compressed size checks code from different places, so don't worry about it. Will resolve once merged.

packages/generator/templates/page/index.tsx Outdated Show resolved Hide resolved
packages/generator/templates/page/index.tsx Outdated Show resolved Hide resolved
packages/generator/templates/page/__modelIdParam__.tsx Outdated Show resolved Hide resolved
@flybayer flybayer changed the title #2023: Give users a Route Manifest Add autogenerated Routes manifest for use with <Link> components Apr 10, 2021
@flybayer flybayer merged commit bf7eed6 into canary Apr 10, 2021
@flybayer flybayer deleted the 2023-route-manifest branch April 10, 2021 20:23
@zanami
Copy link

zanami commented Jul 1, 2021

Would it be better to return { pathname: "/products/[pid]", query: { pid: 123 } } instead? That could make the generated code more legible

It does return that object now and I'm really unsure how to mark link as active based on asPath or pathname from useRouter and this {pathname: 'somepath/[slug]', query {slug:'whatever'}

@Skn0tt
Copy link
Member Author

Skn0tt commented Jul 2, 2021

Hi @zanami! Is this a question you have or an issue you'd like to report? For questions, our Discord works best! For an issue, could you create a new one? I'd love to understand the problem you're facing.

@zanami
Copy link

zanami commented Jul 4, 2021

I'm trying to make ActiveLink work (not using 'as'). It does work if href is a string but I couldn't find an easy way to make it work with href = URL object like { pathname: "/products/[pid]", query: { pid: 123 } }. Can't find any exposed utils to convert it back to string to compare to asPath.
No big deal, I don't have to use Routes as it just adds a bit of convenience.

@itsdillon itsdillon changed the title Add autogenerated Routes manifest for use with <Link> components [legacy-framework] Add autogenerated Routes manifest for use with <Link> components Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate types for /pages routes
4 participants