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] Fix Routes manifest for pages with parent & child relationships (patch) #2424

Merged
merged 5 commits into from
Jun 7, 2021

Conversation

frankiesardo
Copy link

@frankiesardo frankiesardo commented May 30, 2021

@frankiesardo frankiesardo requested a review from flybayer as a code owner May 30, 2021 19:47
@flybayer flybayer changed the title Closes: #2407 Fix Routes manifest for pages with parent & child relationships (patch) Jun 1, 2021
@flybayer
Copy link
Member

flybayer commented Jun 1, 2021

Thanks @frankiesardo! Next time please leave the PR template in place :)

@flybayer flybayer added the 0 - <(^_^)> - merge it! ✌️ Kodiak automerge label Jun 1, 2021
flybayer
flybayer previously approved these changes Jun 1, 2021
@flybayer
Copy link
Member

flybayer commented Jun 1, 2021

@frankiesardo you have a failing test:

[@blitzjs/server::jest] FAIL src/stages/route-import-manifest/route-import-manifest.test.ts
[@blitzjs/server::jest]   ● generateManifest
[@blitzjs/server::jest] 
[@blitzjs/server::jest]     expect(received).toEqual(expected) // deep equality
[@blitzjs/server::jest] 
[@blitzjs/server::jest]     - Expected  - 2
[@blitzjs/server::jest]     + Received  + 2
[@blitzjs/server::jest] 
[@blitzjs/server::jest]     @@ -2,12 +2,12 @@
[@blitzjs/server::jest]         "declaration": "import type { ParsedUrlQueryInput } from \"querystring\"
[@blitzjs/server::jest]       import type { RouteUrlObject } from \"blitz\"
[@blitzjs/server::jest] 
[@blitzjs/server::jest]       export const Routes: {
[@blitzjs/server::jest]         Home(query?: ParsedUrlQueryInput): RouteUrlObject;
[@blitzjs/server::jest]     -   CommentView(query: { postId: string | number; openedCommentPath: (string | number)[] } & ParsedUrlQueryInput): RouteUrlObject;
[@blitzjs/server::jest]     -   UserProfileView(query: { userId: string | number; openedPhotoId?: (string | number)[] } & ParsedUrlQueryInput): RouteUrlObject;
[@blitzjs/server::jest]     +   CommentView(query: { postId: string | number | undefined; openedCommentPath: (string | number | undefined)[] } & ParsedUrlQueryInput): RouteUrlObject;
[@blitzjs/server::jest]     +   UserProfileView(query: { userId: string | number | undefined; openedPhotoId?: (string | number | undefined)[] } & ParsedUrlQueryInput): RouteUrlObject;
[@blitzjs/server::jest]       }",
[@blitzjs/server::jest]         "implementation": "exports.Routes = {
[@blitzjs/server::jest]         Home: (query) => ({ pathname: \"home/\", query }),
[@blitzjs/server::jest]         CommentView: (query) => ({ pathname: \"posts/[postId]/[...openedCommentPath]\", query }),
[@blitzjs/server::jest]         UserProfileView: (query) => ({ pathname: \"users/[userId]/[[...openedPhotoId]]\", query })
[@blitzjs/server::jest] 
[@blitzjs/server::jest]       90 |       },
[@blitzjs/server::jest]       91 |     }),
[@blitzjs/server::jest]     > 92 |   ).toEqual({
[@blitzjs/server::jest]          |     ^
[@blitzjs/server::jest]       93 |     implementation: `
[@blitzjs/server::jest]       94 | exports.Routes = {
[@blitzjs/server::jest]       95 |   Home: (query) => ({ pathname: "home/", query }),
[@blitzjs/server::jest] 
[@blitzjs/server::jest]       at Object.<anonymous> (src/stages/route-import-manifest/route-import-manifest.test.ts:92:5)

Locally run yarn test inside packages/server/ and you'll see the error. Let me know once you fix it

@Skn0tt
Copy link
Member

Skn0tt commented Jun 2, 2021

I don't think it's a good idea to change the signature of the Route Manifest. Why would one of these paths take undefined? If they were actually passed undefined, it'd break.

The better fix here would be to update the generator to emit projectId! instead of projectId.

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

See comment above

@frankiesardo
Copy link
Author

@Skn0tt Yeah that seems a better approach to me 👍

@frankiesardo
Copy link
Author

@Skn0tt I can push a change that adds ! to const __parentModelId__ = useParam("__parentModelId__", "number")! if that works ok with you?

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

Yes, that works @frankiesardo!
One minor thing: useParam actually returns undefined while generating the static site templates, because there are no parameters available at build time. So placing the ! after useParam may confuse devs.
I think we can fix this by placing the ! where they're used as links. I'll push a commit real quick :D

@Skn0tt
Copy link
Member

Skn0tt commented Jun 6, 2021

I can't push to this PR for some reason. Here's the commit: f8e947e

@frankiesardo
Copy link
Author

Yeah makes sense 👍 I've copied that and I've also covered the /new endpoints too

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.

Thank you!!

@flybayer flybayer added 0 - <(^_^)> - merge it! ✌️ Kodiak automerge and removed 0 - <(^_^)> - merge it! ✌️ Kodiak automerge labels Jun 7, 2021
@flybayer flybayer requested review from Skn0tt and removed request for Skn0tt June 7, 2021 16:43
@flybayer flybayer removed the 0 - <(^_^)> - merge it! ✌️ Kodiak automerge label Jun 7, 2021
@flybayer flybayer merged commit 8b245be into blitz-js:canary Jun 7, 2021
@blitzjs-bot
Copy link
Contributor

Added @frankiesardo contributions for code

@itsdillon itsdillon changed the title Fix Routes manifest for pages with parent & child relationships (patch) [legacy-framework] Fix Routes manifest for pages with parent & child relationships (patch) 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.

Blitz generate --parent creates problems with nested routes
4 participants