-
-
Notifications
You must be signed in to change notification settings - Fork 111
feat: convert redirect page to inertia #4076
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
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # lang/en_US.json
wescopeland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do an exhaustive review either later this evening or tomorrow once checks are passing, but wanted to shoot along one quick comment.
# Conflicts: # lang/en_US.json
|
Looks like we've got a coverage gap in Vitest:
PHPUnit is upset about the controller returning Inertia stuff rather than a Blade view. I wonder if |
|
I'll take a look at that, |
# Conflicts: # lang/en_US.json
|
Still looks like we're having some trouble with the PHPUnit test suite: https://github.com/RetroAchievements/RAWeb/actions/runs/19050202919/job/54408281395?pr=4076 Want me to try to poke around? |
|
Yeah I'm not sure where the issue is coming from. These tests run fine locally for me |
|
They all pass for me too. I wonder if this might fix it, would you mind trying this solution? // config/inertia.php
// line 50
resource_path('js/pages'),"Pages" -> "pages". |
I do see the errors on my VM and this prescribed change does make them go away. |
|
I think it might be a case sensitivity issue since I use Mac and it does not have case sensitive folders like Linux (and presumably Windows). |
# Conflicts: # resources/js/ziggy.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's any way we can make this compatible both with raweb.test and localhost:64000. For a moment, I thought something was wrong with my local env 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in one or more git hooks we can auto-run composer types? Though I suppose this may also produce undesirable noise in commits later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with this ziggy.js file, does it need to be committed at all? If we remove it, then ignore it, and keep an ignored file, what happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Every time I pull this branch, it breaks my local (and I'm sure you experience something similar for our branches).
The ziggy.js file is glue between the Inertia app and Laravel's strongly-typed routes. Without it, all routing in the Inertia app breaks.
Our deployment script automatically rebuilds the file on every deploy. I wonder if it would indeed be best to gitignore it and automatically execute php artisan ziggy:generate --types in whatever hooks we need (ie: post-merge).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's possible to kick this off when either pnpm dev or pnpm build run. That may universally solve the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems promising: https://github.com/orgs/pnpm/discussions/5758#discussioncomment-4329215
resources/js/features/redirect/components/+root/RedirectRoot.tsx
Outdated
Show resolved
Hide resolved
resources/js/features/redirect/components/+root/RedirectRoot.tsx
Outdated
Show resolved
Hide resolved
resources/js/features/redirect/components/+root/RedirectRoot.tsx
Outdated
Show resolved
Hide resolved
resources/js/features/redirect/components/+root/RedirectRoot.test.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| // ACT | ||
| const button = screen.getByRole('button', { name: 'Continue to external site' }); | ||
| button.click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feedback will likely be redundant from the button -> a comment I left earlier, but when clicking on things in tests, always best to use await userEvent.click(button). This simulates a real user asynchronously clicking on stuff rather than firing a browser event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to use a userEvent.click() here.
# Conflicts: # lang/en_US.json
Very basic for now. Doesn't change anything, but open to ideas for improvements.
Only notable change is instead of a
<a href>it useswindow.location.replace, which will make pressing "Back" on the browser go to the original page instead of this redirect page.Also, of course, a test and i18n keys.
Redirect's route was moved into an Inertia middleware so it could work, it could be moved elsewhere if needed.
Preview, if relevant.