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

feat: support for basePath #374

Merged
merged 2 commits into from
Jun 3, 2021
Merged

feat: support for basePath #374

merged 2 commits into from
Jun 3, 2021

Conversation

lindsaylevine
Copy link

@lindsaylevine lindsaylevine commented Jun 1, 2021

Fixes #131

note: my first stab at this work included touching every single pageType/redirects.js file (the way we did with i18n) to handle basePath there. with that flowing into several helpers and needing to import the config everywhere again, i opted for the performance cost of needing to loop over all the redirects after theyre collected (if there's a basePath configured) and building a new, basePath-supported collection of redirects. i have my old Logic Everywhere TM commit in a separate branch if anyone's curious lol

remaining issues:

  • catch-all redirects are not working as is (as seen in a run of cypress). not sure the best route here
  • there's a lot of specific logic in here that does stuff that isnt obvious why. could document this better or possibly find a better way

@lindsaylevine lindsaylevine added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jun 1, 2021
/getStaticProps/withFallback/:id /foo/getStaticProps/withFallback/:id 301!
/getServerSideProps/:id /foo/getServerSideProps/:id 301!
/getServerSideProps/all /foo/getServerSideProps/all/:[slug] 301!
/getServerSideProps/all/* /foo/getServerSideProps/all/:[slug] 301!
Copy link
Author

Choose a reason for hiding this comment

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

yeah. uhhhh

@lindsaylevine
Copy link
Author

you absolutely love to see failing tests on one os/node version 💯

@ascorbic
Copy link
Contributor

ascorbic commented Jun 2, 2021

Any idea what's going on here? It seems so strange

@lindsaylevine
Copy link
Author

@ascorbic it has to be the basePathSortFunc on that node version, that's the only thing that could affect the order like this. as for what's wrong with that sorting or includes check on that version, i have no idea 🙃

@lindsaylevine
Copy link
Author

nodejs/node#24294 hmm

@lindsaylevine
Copy link
Author

hm. yeah. its def this sorting issue, because this second try(/commit) failed in a different order than the first. will continue to see how i can eliminate it or make it independent of arg order (like that node issue suggests)

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

🎉

@lindsaylevine lindsaylevine merged commit 8a07038 into main Jun 3, 2021
@lindsaylevine lindsaylevine deleted the ll/feat/base-path branch June 3, 2021 09:50
serhalp pushed a commit that referenced this pull request Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

basePath option is not working
2 participants