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.

Sign up for GitHub

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

Jump to bottom

refactor: render-type to use new_createPages and fix layouts for new_createPages #1000

Merged
merged 7 commits into from
Nov 17, 2024
Merged

refactor: render-type to use new_createPages and fix layouts for new_createPages #1000

merged 7 commits into from
Nov 17, 2024

Conversation

tylersayshi
Copy link
Contributor

@tylersayshi tylersayshi commented Nov 12, 2024 •
edited
Loading

moving e2e test example to use new_createPages

this also fixes the layouts returned from renderRoute to re-use the pathSpec of each route to detect nested layouts.

it also fixes the page mapping issue where the props were not yet being injected and now are. This is for the slugs of the route.

@vercel Vercel
Copy link

vercel bot commented Nov 12, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Nov 15, 2024 6:52am

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Nov 12, 2024 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@tylersayshi
Copy link
Contributor Author

Just realized this is #1000

🎉🍻

@tylersayshi tylersayshi marked this pull request as ready for review November 15, 2024 06:12
@tylersayshi tylersayshi changed the title Refactor e2e test create pages refactor: e2e test to use new_createPages for render-type.spec.ts Nov 15, 2024
tylersayshi
tylersayshi commented Nov 15, 2024
View reviewed changes
packages/waku/src/router/create-pages.ts
pageComponent,
null,
mapping,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi should query and hash also be sent in here?

Copy link
Owner

Choose a reason for hiding this comment

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

If a page is dynamic, I think it should receive query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, and hash only exists on the client, so it is handled inside NewRouter? is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so there's a bug upstream of new_createPages. I think it's upstream of defineRouter too.

This prevents the use of query here. I'll open a new PR to talk through a fix for that, but this PR seems like a fine change on its own to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1004 will fix the query

Copy link
Owner

Choose a reason for hiding this comment

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

thanks. yeah, hash is client only.

@tylersayshi tylersayshi mentioned this pull request Nov 15, 2024
Draft
@dai-shi
Copy link
Owner

dai-shi commented Nov 16, 2024

can you tweak the PR title so that it says it includes a fix?

@tylersayshi tylersayshi changed the title refactor: e2e test to use new_createPages for render-type.spec.ts refactor: render-type to use new_createPages and fix layouts for new_createPages Nov 16, 2024
@tylersayshi tylersayshi mentioned this pull request Nov 16, 2024
Merged
dai-shi
dai-shi approved these changes Nov 17, 2024
View reviewed changes
Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

great to fix it.

@dai-shi dai-shi merged commit 07cd9c0 into dai-shi:main Nov 17, 2024
26 checks passed
dai-shi pushed a commit that referenced this pull request Nov 18, 2024
@tylersayshi
fix: query string should be passed through URLSearchParams (#1004)
bf4fe71 
maybe this is not the right way to fix the issue, but this does change
the query params to be available on the server which was missing before.

needs to come after #1000

this PR should just be for
13a3178
once 1000 is merged

---------

Co-authored-by: Tyler <26290074+thegitduck@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dai-shi dai-shi dai-shi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tylersayshi @dai-shi