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

Add root-path to api-server #1691

Merged
merged 32 commits into from
Apr 1, 2021
Merged

Add root-path to api-server #1691

merged 32 commits into from
Apr 1, 2021

Conversation

jeliasson
Copy link
Contributor

@jeliasson jeliasson commented Jan 28, 2021

Seeks to implement root-path to @redwoodjs/api-server

Related issue

#1693 Add root-path to api-server

@jeliasson jeliasson marked this pull request as draft January 28, 2021 19:52
@github-actions
Copy link

github-actions bot commented Jan 28, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/create-redwood-app-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-api-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-api-server-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-auth-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-cli-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-core-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-dev-server-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-eslint-config-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-eslint-plugin-redwood-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-forms-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-internal-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-prerender-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-router-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-structure-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-testing-0.28.3-257be2d.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1691/redwoodjs-web-0.28.3-257be2d.tgz

Install this PR by running yarn rw upgrade --pr 1691:0.28.3-257be2d

@jeliasson jeliasson mentioned this pull request Jan 28, 2021
3 tasks
@peterp
Copy link
Contributor

peterp commented Mar 12, 2021

Hey @jeliasson, is this ready for review?

@jeliasson jeliasson marked this pull request as ready for review March 12, 2021 09:30
@jeliasson
Copy link
Contributor Author

@peterp Sure, thanks!

@jeliasson jeliasson marked this pull request as draft March 19, 2021 23:05
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Thanks for this! Such a nice and quick PR to review!

@jeliasson jeliasson changed the title Add routePrefix to api-server Add root-path to api-server Mar 22, 2021
jeliasson and others added 2 commits March 22, 2021 22:46
ty @Tobbe

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

This is very very ready to go, I think just passing the rootPath value to http and then we can ship it!

@jeliasson
Copy link
Contributor Author

jeliasson commented Mar 24, 2021

@peterp So two things;

1. Changing back to less-pretty way of handling root path construction

While @Tobbe's suggestion was much more elegant, it did not handle / or nullish. It would result in //. Because of this I reverted to the previous implementation. Do you have something that would make this fly, while being pretty?

2. Using rootPath as option with root-path as alias

Is there a neat way of passing rootPath where the yargs option would be root-path? Currently I'm having rootPath as option, and root-path as alias.

From what I can tell (and tested), this is a working implementation and I'll be happy to make this prettier for 0.29.0 if you want to get this out the door in 0.28.0.. However, if you feel that is not ready - don't hesitate to skip this PR for next sprint.

Also, sorry for my mountain of commits. I should really learn the git rebase stuff. 😅

/cc @thedavidprice @Tobbe

@jeliasson jeliasson requested a review from peterp March 24, 2021 17:15
@thedavidprice
Copy link
Contributor

Release Timing Check-in: we need to bump this to the next release cycle. Please continue with the review. But hold off on merge until post v0.28 release.

@peterp peterp added this to the next release milestone Apr 1, 2021
@peterp peterp merged commit 4ab7a9f into redwoodjs:main Apr 1, 2021
@jeliasson jeliasson deleted the feature/api-server-route-prefix branch April 2, 2021 08:38
@thedavidprice
Copy link
Contributor

@jeliasson For documentation, can you provide an example about how to use this? I'm assuming it's something along the lines of #1693

@jeliasson
Copy link
Contributor Author

For documentation, can you provide an example about how to use this? I'm assuming it's something along the lines of #1693

@thedavidprice Yes. It's implemented as #1693 describes. I can document it on the website, but I'm not entirely sure where this would go. Can you point me in the right direction? 🙏

@thedavidprice
Copy link
Contributor

@jeliasson roger that!

Actually, this is important to work Danny is doing here: #2217

Could you take a look at that PR and make sure the option is correctly included. Then we'll document it all in the CLI Docs. Thanks!

@jeliasson
Copy link
Contributor Author

@thedavidprice Defo. I'll give @dac09 a chat and we get that in there.

This pull request was closed.
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.

4 participants