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

Add types #1557

Closed
wants to merge 2 commits into from
Closed

Add types #1557

wants to merge 2 commits into from

Conversation

koddsson
Copy link
Member

No description provided.

@43081j
Copy link
Contributor

43081j commented Dec 28, 2023

Just FYI so we don't duplicate work again: I also did this in a branch but threw it away to work on a typescript migration instead.

To be clear, don't start migrating to typescript, otherwise this will be the third time we have done the same work in parallel 😂

As a precursor to that, this would be a good one to merge.

@43081j
Copy link
Contributor

43081j commented Dec 28, 2023

Couple of things I did different that may be worth considering:

  • change all occurrences of String to string
  • use typescript JSDoc notation for optional params (e.g. @param {string=} paramName)
  • a few methods that mutate flags and stuff should really still consume an object and cast internally, since flags and friends are internally just stuck onto whatever you give it afaict

@koddsson
Copy link
Member Author

I feel like I've underestimated the amount of work we'd need to do to get types to work :D

Couple of things I did different that may be worth considering:

  • change all occurrences of String to string
  • use typescript JSDoc notation for optional params (e.g. @param {string=} paramName)
  • a few methods that mutate flags and stuff should really still consume an object and cast internally, since flags and friends are internally just stuck onto whatever you give it afaict

Yeah I think we should enforce this with a ESLint plugin.

@43081j
Copy link
Contributor

43081j commented Dec 28, 2023

I feel like I've underestimated the amount of work we'd need to do to get types to work :D

it may be possible to just strongly type the exposed API and cast internally where needed for now.

if you start fixing up all the type errors etc, you're verging on converting it to typescript already. that is exactly what lead me to it - i was fixing up enough types that i realised i may as well be writing typescript... to avoid doing the same effort twice (once in jsdoc, once in type annotations).

im on a different laptop to the one with my typescript branch for the next few days, but once im back on there i'll push what i have

@koddsson
Copy link
Member Author

koddsson commented Jan 24, 2024

Closing this in favor of @43081j PR: #1567

@koddsson koddsson closed this Jan 24, 2024
@koddsson koddsson deleted the add-types branch January 24, 2024 21:16
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

2 participants