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

Investigating CI/Lint type issues #116

Closed
wants to merge 2 commits into from
Closed

Conversation

LukeHagar
Copy link
Collaborator

CI is failing on this PR: sveltejs/kit#12973

Digging into it and running TSC directly in the repo supplies a different set of linting error I'm attempting to address here, but in doing the digging I came across another interesting issue, the build script in this repo introduces a number of TS Linting errors post build.

Running the below commands in this order should demonstrate the issues, and could be causing some of the upstream issues we are seeing.

Standing state of the code

Produces one TS error regarding the eslint config

pnpm i
tsc 

Post-Build state of the code

Produces a number of other errors that will be picked up via
the dist folder in node_modules in other packages when tsc is run

pnpm build
tsc 

I'm not sure that all of this needs to be fully addressed to unblock that PR, but I would love assistance with the error below:

../../node_modules/.pnpm/crossws@0.3.3/node_modules/crossws/dist/index.d.mts(88,19): error TS2315: Type 'Uint8Array' is not generic.

@LukeHagar
Copy link
Collaborator Author

Found this thread with good info, seems to be recently introduced to TS

mozilla/pdf.js#19152

@pi0
Copy link
Member

pi0 commented Feb 3, 2025

I'm not sure node_modules dist should be linted/ only exported types matter.

../../node_modules/.pnpm/crossws@0.3.3/node_modules/crossws/dist/index.d.mts(88,19): error TS2315: Type 'Uint8Array' is not generic.

This line maps to here. I think we should, in first step explicitly declare class method return types to avoid inference at build-time.

@LukeHagar
Copy link
Collaborator Author

Uint8Array ended up being fixed by updating the typescript version, as that generic is only available in 5.7+

@pi0
Copy link
Member

pi0 commented Feb 5, 2025

shall we close this then?

(i think we should anyway do refactor for explicit return types, if happened to have extra time to help, PR is more than welcome!) -- also invited you to repo so you can make PR without forks ❤️

@LukeHagar
Copy link
Collaborator Author

closing this issue for now, thank you!

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