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

[TS] Fix missing default Page type export #11919

Merged

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Jan 22, 2025

As the title says. Fixes the last TypeScript error in packages/router/src/__tests__/pageLoadingContext.test.tsx.

@Philzen
Copy link
Contributor Author

Philzen commented Jan 22, 2025

@Tobbe do you think we could easily introduce a typecheck job (similar to yarn rw tc in RW projects) into the CI and start fixing & including files and folders incrementally?

Not sure if there were any rules of engagement regarding this approach in the past, the downside here is of course slightly increased CI runtime and a slower iteration cycles. But the advantages could be worth it, especially we could never break working and tested types again in new releases – thinking of all the commits that were just rolled back in the Router Set type for example, which would never have been merged in the first place if such a job was in place.

@Philzen
Copy link
Contributor Author

Philzen commented Jan 22, 2025

@Tobbe do you think we could easily introduce a typecheck job (similar to yarn rw tc in RW projects) into the CI and start fixing & including files and folders incrementally?

Ahh it looks like there already exists something like that:

grafik

However it apparently does not check the tests folder – which i believe should be included as they are the actual application of the types.

@Tobbe
Copy link
Member

Tobbe commented Jan 24, 2025

However it apparently does not check the tests folder – which i believe should be included as they are the actual application of the types.

The team was divided on TS tests. Some didn't think tests should be written using TypeScript at all. But I'm all for TS and type tests. So I agree!

@Tobbe Tobbe added the release:chore This PR is a chore (means nothing for users) label Jan 25, 2025
@Tobbe Tobbe added this to the chore milestone Jan 25, 2025
@Tobbe Tobbe merged commit fdb0937 into redwoodjs:main Jan 26, 2025
50 of 54 checks passed
@Tobbe
Copy link
Member

Tobbe commented Jan 26, 2025

@Philzen I merged this, but would be very interested in another PR that adds type tests (and better TS support over all) for our test

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