Skip to content

Conversation

@JamieMagee
Copy link
Contributor

@JamieMagee JamieMagee commented Jul 4, 2025

The majority of these types were straightforward to add, and should be self-explanatory to review. However, there were a few places where I had to workaround some issues. I've left comments at the relevant places in the code.

Other than that, I also:

  • Added @tsconfig/strictest
  • disabled strictNullChecks as it's not really possible to enforce in JavaScript
  • Added custom types for:
    • @clearlydefined/spdx as it doesn't ship with them (I will contribute then back to clearlydefined/spdx afterwards)
    • painless-config as the types it ships with are incorrect and the upstream repository is archived
    • winston-azure-application-insights as it doesn't ship with them

@JamieMagee JamieMagee force-pushed the more-types branch 5 times, most recently from 454a2c9 to a735397 Compare July 5, 2025 07:44
@JamieMagee JamieMagee marked this pull request as ready for review July 6, 2025 02:50
@JamieMagee JamieMagee changed the title [WIP] Additional types Additional types Jul 6, 2025
@JamieMagee JamieMagee requested a review from qtomlinson July 7, 2025 15:31
@jeffmendoza
Copy link
Member

It would be good to have a docs/typescript-usage.md doc that covers

  • Where we are in the referenced "migration" documentation
  • What we have decided to do and not do
  • What the current usage is.

JamieMagee added a commit to JamieMagee/spdx that referenced this pull request Jul 9, 2025
@JamieMagee
Copy link
Contributor Author

JamieMagee commented Jul 9, 2025

It would be good to have a docs/typescript-usage.md doc that covers

* Where we are in the referenced "migration" documentation

* What we have decided to do and not do

* What the current usage is.

Good idea! But I'll handle that in a separate PR. This one has already grown quite large.

EDIT: @jeffmendoza ready for review #1336

Copy link
Collaborator

@qtomlinson qtomlinson left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed and excellent work! Several of my comments focus on the declaration of private properties or methods in different parts of the code. Let me know if you need any clarification.

private cache: any

/** Default TTL in seconds for cached items */
private defaultTtlSeconds: number
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider omitting private properties from the declaration, cache and defaultTtlSeconds?


/** Promise for client initialization to prevent multiple concurrent initializations */
private _clientReady: Promise<void> | null

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider omitting private properties from the declaration, _client and _clientReady?

@JamieMagee
Copy link
Contributor Author

@qtomlinson Thank you for the review. I think I've addressed all your comments. The ones I haven't addressed are related to adding types for private methods. I'll try and reply to all of those here, instead of each comment individually.

It's not necessary to add types for private fields and methods, but I did it for a few reasons:

  • Future-proofing for a TypeScript migration
  • Better tooling and IDE integration
  • I was already here, and it wasn't much additional effort 😅

@JamieMagee JamieMagee requested a review from qtomlinson July 11, 2025 03:03
Copy link
Collaborator

@qtomlinson qtomlinson left a comment

Choose a reason for hiding this comment

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

@JamieMagee Thank you for incorporating the review comments.

Declaring private fields or methods within the types file can reduce readability. For instance, the GradleCoordinatesMapper class contains only two public methods:

map(coordinates: GradleCoordinates): Promise<EntityCoordinates | null>
getMavenMetadata(pluginId: string): Promise<string | FetchResponse<string> | null>

This is easier to understand compared to a scenario where there are 2 public and 8 private methods.

Nonetheless, this is not a critical issue. I appreciate the detailed and excellent changes you've made!

@JamieMagee JamieMagee merged commit 5ccfb3b into master Jul 11, 2025
4 checks passed
@JamieMagee JamieMagee deleted the more-types branch July 11, 2025 15:57
JamieMagee added a commit that referenced this pull request Jul 11, 2025
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.

4 participants