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 TS types for Check, remove circular dependency #11901

Merged
merged 3 commits into from
Mar 29, 2024
Merged

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Mar 26, 2024

Description

Adds types for the Check group of functions with type assertions to help in TS based projects.
I chose to add this as a Check.d.ts file right next to Check.js to keep them closer together. tsd-jsdoc doesn't seem to have a way to import this types file though so it still needs a custom replace in our generate TS build step.

Also removed a circular dependency I accidentally added in #11859

Issue number and link

No issue

Testing plan

  • Run npm run build-ts and make sure it succeeds
  • Check Source/Ceisum.d.ts and packages/engine/index.d.ts that they have the correct definition for Check
  • Run npm run build-docs and make sure it succeeds
  • Check that the Documentation now has Check on the globals page.

For the circular dependency you can use dpdm to check in main and this branch with the command npx dpdm packages/engine/Source/DataSources/CzmlDataSource.js

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz March 26, 2024 17:10
Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

)
.replace(
/\/\*\*[\*\s\w]*?\*\/\nexport const Check: any;/m,
`\n${readFileSync("./packages/engine/Source/Core/Check.d.ts").toString()}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be made generic to and **/Source/**/*.d.ts file? That way any future additions don't need to tweak build code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to do that but I couldn't find or think of a nice clean way to do that. As it is it needs to be specific to replace the definition in the types file for the corresponding code. Otherwise we'd end up with a duplicated definition which breaks TS. I'd love if tsd-jsdoc let you pass in a set of types files to use in places like this.

* @param {string} name The name of the variable being tested
* @param {*} test The value that is to be checked
* @exception {DeveloperError} test must be defined
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why the inline docs need to be duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without them the intellisense that VSCode picks up has no description of any of the functions. The way the build is adding this it's taking the file wholesale. I was not able to find a way to get the tsd-jsdoc to output anything other than const Check: any so it also didn't include any of the descriptions other than the top one for Check. It's annoying and I'd love to remove the duplication but I couldn't figure out a way to do that and I didn't want to spend a ton more time on digging into it.

@ggetz
Copy link
Contributor

ggetz commented Mar 27, 2024

I have updated CHANGES.md with a short summary of my change

Technically speaking, this should be added as an addition in CHANGES.md.

Copy link
Contributor

@jjhembd jjhembd left a comment

Choose a reason for hiding this comment

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

Thanks @jjspace, this looks like a reasonable workaround for now.
ts-jsdoc is a little messy, as we have noticed in #10455, and the Check object itself could use some refactoring. But until we get to those bigger projects, this works to generate the needed TS types.

@jjhembd jjhembd merged commit 432e88a into main Mar 29, 2024
9 checks passed
@jjhembd jjhembd deleted the add-check-types branch March 29, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants