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

refactor(core): Better typescript support #345

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

Conversation

Myphz
Copy link

@Myphz Myphz commented May 29, 2024

  • Added a better tsconfig
  • Fixed all typescript errors in core/index.ts
  • Use narrower types, avoid anys

This PR should not affect the logic whatsoever. It should improve the DX for TypeScript users.

@Myphz Myphz requested a review from a team as a code owner May 29, 2024 20:34
@Myphz Myphz requested review from feledori and GFier May 29, 2024 20:34
Copy link

vercel bot commented May 29, 2024

@Myphz is attempting to deploy a commit to the darkroom Team on Vercel.

A member of the Team first needs to authorize it.

@Myphz Myphz mentioned this pull request May 29, 2024
@feledori
Copy link

hey, thanks for taking the time on this! i am not sure we want to have all the typescript guards in the implementation. The typescript types are important for the api consumer not internals. a good choice may be defining types in a d.ts and leaving the core in js with jsdoc. I don't think @clementroche would like to use type guard like isHTMLElement. Will look review it closer when I have time for it. thanks tho.

@clementroche
Copy link
Member

clementroche commented May 30, 2024

hello @Myphz, thank you for your PR however i've just merged the snap branch into main. Can you adapt your PR accordingly ?

@Myphz
Copy link
Author

Myphz commented May 30, 2024

hey, thanks for taking the time on this! i am not sure we want to have all the typescript guards in the implementation. The typescript types are important for the api consumer not internals. a good choice may be defining types in a d.ts and leaving the core in js with jsdoc. I don't think @clementroche would like to use type guard like isHTMLElement. Will look review it closer when I have time for it. thanks tho.

Thanks for your feedback. My changes were mostly on the index.ts file and the Lenis class definition, which I believe is what is consumed by the APIs, not internally. Since that file was already using TypeScript, I figured it'd be helpful to add good type definitions to it. I didn't touch any other JavaScript file.

I will soon update the PR for the latest update. Let me know if there's a better way to handle this task. Thank you!

@feledori
Copy link

hey, thanks for taking the time on this! i am not sure we want to have all the typescript guards in the implementation. The typescript types are important for the api consumer not internals. a good choice may be defining types in a d.ts and leaving the core in js with jsdoc. I don't think @clementroche would like to use type guard like isHTMLElement. Will look review it closer when I have time for it. thanks tho.

Thanks for your feedback. My changes were mostly on the index.ts file and the Lenis class definition, which I believe is what is consumed by the APIs, not internally. Since that file was already using TypeScript, I figured it'd be helpful to add good type definitions to it. I didn't touch any other JavaScript file.

I will soon update the PR for the latest update. Let me know if there's a better way to handle this task. Thank you!

yeah, makes sense. maybe a seperate d.ts file would make more sense. will have to talk this over. types seem mostly sound as far I can tell from the quick glance i took. good job.

@Myphz
Copy link
Author

Myphz commented Jun 3, 2024

I've updated the PR to the latest branch. This time, I made some changes to the Lenis class to simplify it a bit:

  • Removed userData parameter and this.userData (apparently it was useless - it was never assigned)
  • this.isTouching removed (it was assigned but it was never used)
  • Removed this.hasScrolled (it was used but never assigned, so it always resulted in undefined - the assignment was commented)

I may be wrong on these assumptions though - let me know your thoughts. Thanks

@clementroche
Copy link
Member

userData is necessary

@Myphz
Copy link
Author

Myphz commented Jun 4, 2024

userData is necessary

fixed

@clementroche
Copy link
Member

Thank you @Myphz, I will not merge your PR but use it as a base for improving types step by step to be sure that it doesn't break nothing and that I understand every choice you've made.

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.

None yet

3 participants