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

chore(typechecking): typecheck /lib #1138

Merged
merged 18 commits into from
Sep 9, 2024
Merged

chore(typechecking): typecheck /lib #1138

merged 18 commits into from
Sep 9, 2024

Conversation

pdesoyres-cc
Copy link
Contributor

@pdesoyres-cc pdesoyres-cc commented Aug 2, 2024

what is this PR

This PR adds typechecking on almost all files in /lib.

There are still TS issues when importing modules from clever client.
This prevents us from including all /lib directory in typecheck:ci task.

todo

@pdesoyres-cc pdesoyres-cc changed the title More typechecking chore(typechecking): typecheck /lib Aug 2, 2024
@pdesoyres-cc pdesoyres-cc added the maintenance Code refactoring, project structure, dev tooling (storybook, dev server, npm tasks...) label Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/more-typechecking/index.html.

This preview will be deleted once this PR is closed.

Copy link
Contributor

@florian-sanders-cc florian-sanders-cc left a comment

Choose a reason for hiding this comment

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

Gg @pdesoyres-cc this is great!

I've only added a few nitpicks & some suggestions to avoid a few @ts-ignore but apart from that it's all good.

I must admit I did not have enough time to review the smart component part thoroughly so I'll get back to that when I come back if it's not already merged.

src/lib/fake-strings.js Outdated Show resolved Hide resolved
src/lib/date/date-utils.js Outdated Show resolved Hide resolved
src/lib/date/date-formatter.js Outdated Show resolved Hide resolved
src/lib/ansi/ansi-palette-analyser.js Outdated Show resolved Hide resolved
src/lib/ansi/ansi-palette-analyser.js Outdated Show resolved Hide resolved
src/lib/ansi/ansi-palette-analyser.js Show resolved Hide resolved
src/lib/send-to-api.js Outdated Show resolved Hide resolved
@pdesoyres-cc pdesoyres-cc force-pushed the more-typechecking branch 2 times, most recently from a60b0dd to e23f12f Compare September 2, 2024 15:56
Copy link
Contributor

@HeleneAmouzou HeleneAmouzou left a comment

Choose a reason for hiding this comment

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

Well done 👏 I did not review the smart component relative files and I focused on the other files. I have no comments to make, but I have discovered unknown ways of typechecking, thanks ! I will come back later when the other will have review the PR, considering the subject I think it might be interesting for me to read the reviews from the team.

Copy link
Member

@hsablonniere hsablonniere left a comment

Choose a reason for hiding this comment

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

Impressive work @pdesoyres-cc, well done!!!

 There are still TS issues when importing modules from clever client.

It seems like there will be improvements on this soon ™️ CleverCloud/clever-client.js#57 (comment)

Really nice that you added function descriptions in the JSDoc!!!

This prevents us from including all /lib directory in typecheck:ci task.

Can we add the files that don't have this problem, one by one?

@florian-sanders-cc
Copy link
Contributor

This prevents us from including all /lib directory in typecheck:ci task.

  Can we add the files that don't have this problem, one by one?

We could use // @ts-expect-error FIXME: remove when clever-client exports types above every clever-client import: https://github.com/CleverCloud/clever-components/pull/1136/files#diff-c825a5f405ed2499499eb16a999c4060e9176b5561c57108c2a5c55ea100b3bdR1

This allows us to typecheck the rest of the file and when the client exposes types, the @ts-expect-error will be flagged as useless by the compiler so we'll get errors telling us to remove them 😉

Copy link
Contributor

@florian-sanders-cc florian-sanders-cc left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @pdesoyres-cc 😎

@pdesoyres-cc pdesoyres-cc merged commit a0b2671 into master Sep 9, 2024
4 checks passed
@pdesoyres-cc pdesoyres-cc deleted the more-typechecking branch September 9, 2024 14:03
Copy link
Contributor

github-actions bot commented Sep 9, 2024

🔎 The preview has been automatically deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Code refactoring, project structure, dev tooling (storybook, dev server, npm tasks...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants