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

Perseus rocket integration #266

Merged
merged 13 commits into from
Apr 6, 2023
Merged

Conversation

Miroito
Copy link
Contributor

@Miroito Miroito commented Mar 7, 2023

This is a rocket integration for perseus.

I have mainly tried to translate the other integrations into the rocket structure.

Here are a few notes I think are worth pointing out:

  • I made the decision to return a rocket base app instead of routes in the default function (usually called perseus_routes), I find it more convenient for this case but it is probably possible to return a list of routes as long as the developer that will write a custom server later remembers to manage the variable opts. This constraint can be worked around the same way I worked around the issue with the turbine containing generics.
  • I'm not very confident that it will work with all perseus features right away, since I lack knowledge on the insides of perseus, I have done some smoke tests though with build paths and incremental generation. I have not tried to use translations.

I'm not explaining the details of the code so you can have a clean view as a reviewer. That being said, I will happily explain and discuss my decisions when prompted.

Lastly, I hope the current CI is able to test if this integration is fully compatible with perseus. I'm open to feedback there, maybe something I could write that would make sure this integration works now and for the future.

@arctic-hen7
Copy link
Member

arctic-hen7 commented Mar 9, 2023

Thank you so much for this! You should be able to integrate it into the CI system quite simply, by adding this line to the bottom of the script in bonnie.toml with the key test.subcommands.example-all-integrations.cmd (line 197 I think):

"EXAMPLE_INTEGRATION=rocket bonnie dev example %category %example test"

That will add the rocket integration to all the tests, except those that are integration-locked, which will behave as you'd expect.

You'll need to use bonnie test though to run the tests manually, since I'm pretty sure they won't integrate for this PR.

@Miroito
Copy link
Contributor Author

Miroito commented Mar 9, 2023

Thanks for the feedback.

I have run the tests, and everything passes for me with this latest commit.

The initial load handler route that matches everything now has a rank of 100 instead of 0 because FileServer rocket routes default to 10. And that's the only change done to actual behavior.

Edit: The CI is completely broken because of one package: cargo_toml :(

@Miroito
Copy link
Contributor Author

Miroito commented Mar 11, 2023

Rebased on main to fix the cargo_toml issue.

Edit:
i18n fails on the rocket integration, I can't reproduce this issue, the test passes on my machine. Any idea what's going on?
Capsules fails on the warp integration. Same as before, all tests pass on my machine... weird.

I've tried to delete my Cargo.lock and let cargo create it again but the tests still pass
My thinking was that this divergence comes from the fact that Cargo.lock is not committed, there are a surprising amount of differences between the previous lock file and the one I re-generated now.

@arctic-hen7
Copy link
Member

If everything is passing on your machine, that is fine. The i18n test is temperamental to say the least, and capsules sometimes fails as well. I usually just re-run them and wait for them to work. If it fails three times, I run locally. I will get around to automating the re-running of failing tests at some stage...\

I'm just releasing a new version to fix cargo_toml dependence (since that's breaking all new installations of the CLI), and then I'll merge this.

@Miroito
Copy link
Contributor Author

Miroito commented Mar 12, 2023

I just realized I forgot to figure out how to give the host parameter to Rocket.

Maybe there are more things I forgot about, please don't hesitate to look thoroughly and I'll make the changes.

@Miroito
Copy link
Contributor Author

Miroito commented Mar 21, 2023

Took the time to run the tests today with the latest commits and everything is successful locally.

Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Just a few minor suggestions and this should be good to merge! Thank you so much for all your hard work!

P.S. Sorry for taking so long to get to this properly, I've been very busy the last few weeks.

examples/core/custom_server_rocket/Cargo.toml Outdated Show resolved Hide resolved
examples/core/custom_server_rocket/README.md Outdated Show resolved Hide resolved
examples/core/custom_server_rocket/src/main.rs Outdated Show resolved Hide resolved
examples/core/custom_server_rocket/src/templates/index.rs Outdated Show resolved Hide resolved
packages/perseus-rocket/Cargo.toml Outdated Show resolved Hide resolved
packages/perseus-rocket/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-rocket/src/lib.rs Outdated Show resolved Hide resolved
packages/perseus-rocket/README.md Show resolved Hide resolved
@Miroito
Copy link
Contributor Author

Miroito commented Apr 3, 2023

I've made all the changes. The CI fails in ways that don't seem related to the changes I made. I have not taken the time to run any local tests to confirm though.
Thanks for the thorough review, I really appreciate it. It shows you actually read the code which is not the case everywhere I've contributed.
For the future, I'm really interested in middleware/layer features, it's the last thing (I think) that keeps me from going to a perseus-only stack. Hit me up if you see an opportunity where I could help.

Copy link
Member

@arctic-hen7 arctic-hen7 left a comment

Choose a reason for hiding this comment

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

Just a few very tiny things (one that I missed in the last pass).

Also thank you for your kind words, I try to make sure I really understand any code that's contributed to my projects, because I know that, at some stage, I'll probably have to wrangle with it! As for middleware, that's all very much theoretical right now, but I thank you very much for the offer!

packages/perseus-rocket/README.md Outdated Show resolved Hide resolved
packages/perseus-rocket/src/lib.rs Outdated Show resolved Hide resolved
@arctic-hen7
Copy link
Member

Also I wouldn't worry about the CI for now, I'll sort that out on my end once this is merged. Most things are clearly working, and the CI is, as I said earlier, temperamental.

@Miroito
Copy link
Contributor Author

Miroito commented Apr 5, 2023

There you go, I made many more than 2 passes and did not catch those issues, hopefully it's the last that remains from my careless copy pasting.

I believe some of those links (the perseus repo link mainly) are outdated in other packages, I have copied basically all comments from the axum or warp integration (I don't remember which) and they still mention your own repo (not framesurge). It's out of scope for this PR though so I did not make any changes there.

@arctic-hen7
Copy link
Member

No problem! Yeah, I did a find-and-replace a long time ago to fix those which seemed to miss a few (dozen) files, which have slowly cropped up over time. There are redirects all in place so it's not a big deal, but I'll take a closer look and see where the leftovers are throughout the repo. With all that done, I think this is good to merge!

Once again, thank you so much for all your hard work on this, it really is greatly appreciated!

@arctic-hen7 arctic-hen7 merged commit 25bd115 into framesurge:main Apr 6, 2023
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.

2 participants