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

Initial typings fs-router folder #709

Merged
merged 16 commits into from
Feb 9, 2023
Merged

Conversation

birkskyum
Copy link
Contributor

@birkskyum birkskyum commented Feb 3, 2023

It's hard to dive in and debug the FileRouter ( #707 ), because the fs-router folder doesn't have types.

This PR converts some of the fs-router files to typescript, without breaking the tests, and adds some types as a starting point.

The "typecheck" on the start package is a bit problematic, and should maybe be temporarily disabled, as it make a gradual migration to typescript very hard - because it will break until a file is 100% typed. I did partial typings of a few more files and had to revert it to .js to make the typecheck pass.

@birkskyum birkskyum changed the title Initial typings fs router Initial typings fs-router folder Feb 3, 2023
@@ -1,7 +1,21 @@
export default function fileRoutesImport({ types: t }) {
// @ts-ignore
Copy link

Choose a reason for hiding this comment

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

Why @ts-ignore has been added here?

Copy link
Contributor Author

@birkskyum birkskyum Feb 7, 2023

Choose a reason for hiding this comment

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

Otherwise it'll say:

Cannot find module '@babel/types/lib' or its corresponding type declarations.ts(2307)

Copy link

@aminya aminya Feb 8, 2023

Choose a reason for hiding this comment

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

Doesn't @types/babel__types have the types? I think by installing this package and importing things from @babe/types the issue would be resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have another look at this

@aminya
Copy link

aminya commented Feb 5, 2023

The "typecheck" on the start package is a bit problematic, and should maybe be temporarily disabled, as it make a gradual migration to typescript very hard - because it will break until a file is 100% typed. I did partial typings of a few more files and had to revert it to .js to make the typecheck pass.

I guess it makes sense to ignore the errors using @ts-ignore until the issues are fixed. The TypeScript errors are also affecting the end-users, so the migration should be done (see #255).

@ryansolid ryansolid merged commit 1890f44 into solidjs:main Feb 9, 2023
@ryansolid
Copy link
Member

ryansolid commented Feb 10, 2023

This code runs in node.. unfortunately which means it chokes on ts trying to do local builds as it doesn't get transformed and ends up breaking stuff. I have no clue how tests are passing but I think I will need to revert this unfortunately for now.

ryansolid added a commit that referenced this pull request Feb 10, 2023
birkskyum added a commit to birkskyum/solid-start that referenced this pull request Mar 9, 2023
* type path-utils

* convert fileRoutesImport to ts

* add @types/babel-types

* add types to fileRoutesImport

* use @babel/types directly

* add @babel/types as dependency

* convert manifest to ts

* add some types to manifest

* rename router.js to .ts

* convert router.js to .ts

* Revert "rename router.js to .ts"

This reverts commit 38049e1.

* Revert "add some types to manifest"

This reverts commit 9e44f91.

* Revert "convert manifest to ts"

This reverts commit 23ce4a3.

* ts-ignore @babel/types

* revert router.js

---------

Co-authored-by: Ryan Carniato <ryansolid@gmail.com>
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.

3 participants