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

Continues correcting JSDoc types for Typescript tooling compatibility #58

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

ChrisC
Copy link
Contributor

@ChrisC ChrisC commented Jul 9, 2024

No description provided.

@ChrisC
Copy link
Contributor Author

ChrisC commented Jul 9, 2024

@jugglinmike there are two clusters of files remaining that would complete the typescript formalization work:
shared/job and the source and test files for shared/file-record.

I anticipate that fixing the types for both of these modules will be a bit difficult and probably require some amount of re-factoring. That being the case, I'm wondering if we might want to hold off on correcting these types for now, since a bigger refactor to simplify the harness is coming.

In particular, I'm wondering if we might want to do away with the shared/jobs abstraction altogether? I'm a little less clear on whether or not we may need the file-record abstraction going forward, but I do think this module would benefit from a substantial simplification if we do decide to keep it.

@jugglinmike @gnarf It might be a good idea for us to plan out that bigger refactor in a little more detail, so that we can make sure to only correct the types we know we'll continue using after the refactor?

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

@jugglinmike @gnarf It might be a good idea for us to plan out that bigger refactor in a little more detail, so that we can make sure to only correct the types we know we'll continue using after the refactor?

Agreed. We prioritized this work because we'd have more confidence refactoring a well-typed project. There's no sense spending a lot of time fixing doomed files.

src/shared/iterate-emitter.js Outdated Show resolved Hide resolved
@ChrisC
Copy link
Contributor Author

ChrisC commented Jul 10, 2024

Agreed. We prioritized this work because we'd have more confidence refactoring a well-typed project. There's no sense spending a lot of time fixing doomed files.

Got it. In that case, I'll go ahead and submit this PR and we'll determine later whether or not those last files will make it into the upcoming refactor.

@ChrisC ChrisC marked this pull request as ready for review July 10, 2024 17:49
Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Thanks, Chris!

@jugglinmike jugglinmike merged commit 2dacb5e into w3c:main Jul 10, 2024
4 checks passed
@jugglinmike jugglinmike deleted the correct-types-final-pass branch July 10, 2024 20:19
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