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

[Tests] Amend typos and extraneous <Set /> generic typings in router tests #11894

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Jan 10, 2025

Just a handful of test name and typescript amendments – discovered while skimming over https://github.com/redwoodjs/redwood/pull/10464/files

@Philzen Philzen changed the title [test] Amend typos and <Set /> typings [Tests] Amend typos and <Set /> typings Jan 10, 2025
@Philzen
Copy link
Contributor Author

Philzen commented Jan 10, 2025

@Tobbe i'm afraid you're not going to like this (packages/router/src/tests/router.test.tsx):

image

#11739 was still fine – it's also what i would love to see in the next v8 patch release as it fixes what broke during the migration from 7 → 8.

Starting with #11756 the typing all went downhill 🙈
Issue there

  • It was in theory a nice idea of me to use keyof AvailableRoutes for autocompletion, but poorly executed as of course it only worked with my locally generated types in the .redwood folder. So any route name occurences would either have to be @ts-ignored ignored now in the main project or we'd need to find another way to generate those route typings.
  • Then this change (i believe) then broke the nested types

Not sure if this is easily fixable or if we maybe should revert starting from #11756 for the time being and try another day. Just wanted to quickly share this as a heads up.

@Tobbe
Copy link
Member

Tobbe commented Jan 15, 2025

Arrrg! Router typings! What a mess 🤬 Why is this to difficult to get right 😰

I'm down to revert if we think that's the best way forward

@Tobbe
Copy link
Member

Tobbe commented Jan 15, 2025

Trying to figure out what PRs to revert

#11756
#11767
#11769

@Philzen Does that look like the complete list to you?

@Philzen
Copy link
Contributor Author

Philzen commented Jan 15, 2025

@Philzen Does that look like the complete list to you?

grafik

I just double-checked – everything after #11739 (which is OK) adds 8 red squiggles to packages/router/src/tests/router.test.tsx. So for revert that would be

#11769
#11768
#11767
#11756

… in that order of revert should work.

Why is this to difficult to get right 😰

Something about AvailableRoutes apparently is more complicated than it looks – at least that is the PR where things went south.

I'm down to revert if we think that's the best way forward

Well, at least we have a document of our joint failure in the git history now 🤣 – we can always start over from there.
Going forward, i feel it is vital to include TS typecheck into the CI to avoid this kind of disappointing hindsight surprises. I realize it's impossible to get the whole repo to pass out-of-the-box, so we should start with the ones that are safe, fixing one by one and including them in the TS check job as we go along.

Then we would at least be sure typings can only get better and never get worse / broken again 😎

@Philzen Philzen changed the title [Tests] Amend typos and <Set /> typings [Tests] Amend typos and extraneous <Set /> generic typings Jan 15, 2025
@Philzen Philzen changed the title [Tests] Amend typos and extraneous <Set /> generic typings [Tests] Amend typos and extraneous <Set /> generic typings in router tests Jan 15, 2025
@Tobbe
Copy link
Member

Tobbe commented Jan 17, 2025

PRs have been reverted

#11902 reverts #11769
#11903 reverts #11768
#11904 reverts #11767
#11905 reverts #11756

@Tobbe Tobbe added the release:chore This PR is a chore (means nothing for users) label Jan 17, 2025
@Tobbe Tobbe added this to the chore milestone Jan 17, 2025
@@ -653,7 +653,7 @@ test('Nested sets, and authentication logic', () => {
})
})

test('Give correct ids to root sets', () => {
test('gives correct ids to root sets', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I actually think this was better the way it was.
Had this been an it-test instead I would have agreed with your change. But with test I think "test: Give correct..." reads better

(But I really wish all tests used it())

@@ -925,11 +925,7 @@ test('Private is an alias for Set private', async () => {
const TestRouter = () => (
<Router useAuth={mockUseAuth({ isAuthenticated: true })}>
<Route path="/" page={HomePage} name="home" />
<Private<PrivateLayoutProps>
Copy link
Member

Choose a reason for hiding this comment

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

With all the type change PRs reverted I think I want to keep this as it was. What's your thought on that, @Philzen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the end of the day, these are completely redundant – there is no more or less TS errors in these files with or without them.

But let me double check if it changes anything regarding auto-completion – although even then, they would be just noise in the test, as they don't serve any purpose concerning stuff that would be able to come up in a CI.

My thought is to find a use case where they are actually required and implement a test for that, whilst removing those generics where they aren't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants