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

Use vendored next code to prepare view path #96

Merged

Conversation

yakovlev-alexey
Copy link
Contributor

Should fix #93

Additional testing is needed:

  • with fastify
  • any route (e.g. [...slug]) does not work (probably never did)
  • with next@11
  • with next@13

Tested in nest-next-example branch - https://github.com/yakovlev-alexey/nest-next-example/tree/next-12-fix-routing

Perhaps some caching can be done to prevent recalculating regexes. Static paths may omit regex calculations and path interpolation by checking if the path is dynamic (Next code for this check). A part of me wonders whether implementing our own path interpolation function is more effective.

Seems as if dynamic routing never worked since Next@12 therefore there really is not much hurry. I'd first implement a test suite and only then go on with this PR.

next directory contains code copied from Next sources. Should add permalinks to the same code in next.js repo.

@kyle-mccarthy
Copy link
Owner

Hey thanks for the contribution. Would you be able to add some tests for pre + post dynamic path interpolation? I am also curious about other interpolation methods, I know that radix trees have exceptional performance when used for this kind of routing.

@kyle-mccarthy
Copy link
Owner

Another idea, can we leverage the routes-manifest.json file that next generates on build?

@yakovlev-alexey
Copy link
Contributor Author

Actually that is possible. I will give it a try

@yakovlev-alexey
Copy link
Contributor Author

Updated to use dynamicRoutes property in Next server. Seems like a cleaner solution (though the property is untyped) Ran tests in https://github.com/yakovlev-alexey/nest-next/actions/runs/3426869423 - seems fine.

@kyle-mccarthy what do you think about merging #97 first so that CI is run in this repo?

@kyle-mccarthy
Copy link
Owner

Hey this looks pretty awesome. I have been out of town for work and haven't had a chance to review this yet. I just wanted to let you know that it is on my radar and I will get to it ASAP!

@yakovlev-alexey
Copy link
Contributor Author

Hey @kyle-mccarthy! Any updates on this?

@yakovlev-alexey
Copy link
Contributor Author

Hey @kyle-mccarthy! It has been a few months, do you want to proceed with this PR (and the test suite one)? If you are missing the time required to maintain this package you may include me as a maintainer in GitHub repo.

@kyle-mccarthy
Copy link
Owner

@yakovlev-alexey hey I merged in the other PR but it looks like there are some errors in CI. would you be able to look into them? once we get CI in a good state I will merge this one in too. thanks for your contributions, I really appreciate it!

@yakovlev-alexey
Copy link
Contributor Author

Actually CI errors reflect the issue in #93. All errors should be fixed in this PR. I think you should see a button to run Actions on this PR. If you do not I guess actions configuration should be changed a bit to allow runs on forks. In the meantime you can refer to the mirror PR in my own fork - yakovlev-alexey#2. CI for this branch there is green.

@yakovlev-alexey
Copy link
Contributor Author

@kyle-mccarthy ping

@kyle-mccarthy
Copy link
Owner

@yakovlev-alexey would you be able to update the README to document the new features and remove any existing documentation that would no longer be true once this PR is merged. If any diagrams are required we can use excalidraw.com. thanks!

@yakovlev-alexey
Copy link
Contributor Author

@kyle-mccarthy. Did not really find any new actual features to document. Realised I never left any documentation or a command to run tests so did that. Also added a skipped test and known issues section to reflect the remaining issue with "catch all routes" pages.

@RenXusheng233
Copy link

Hi, I want to know when will this PR merge? THX.

@kyle-mccarthy kyle-mccarthy merged commit a9e2651 into kyle-mccarthy:master Jan 30, 2023
@kyle-mccarthy
Copy link
Owner

@yakovlev-alexey These changes have been merged and are in the 10.1.0 release, thank you for your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nest routing issue with nested pages
3 participants