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

DX: Fix defined and Check types for CesiumJS devs #12312

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Nov 15, 2024

Description

  • Add defined.d.ts which will now get picked up and provide the correct type predicates and assertions so JS Type checking will understand things are defined after this call
Before After
2024-11-15_16-11 2024-11-15_16-11_2
Before After
2024-11-15_16-14 2024-11-15_16-19
  • Updated the build process to account for these files and changes. The built Cesium.d.ts and engine/index.d.ts files should be identical to the previous ones.

Issue number and link

No issue, just a small DX issue I've grown too tired of seeing

Testing plan

  • In main run npm run built-ts
  • Create a copy of Source/Cesium.d.ts and packages/engine/index.d.ts somewhere you can check later
  • Switch to this branch and run npm run build-ts
  • Do a diff of those files and ensure they are identical. This confirms our external use will be the same downstream
  • To test locally find a usage of defined and Check locally
  • Turn on the VSCode setting Check JS for type checking with the TS server (This may or may not be necessary, I'm not sure)
  • Verify that in main the types aren't correct as shown above, verify that in this branch they are
  • Turn off CheckJS if you're not ready to see a lot of other (false positive) errors scattered in our code

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 updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjspace!

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

Comment on lines +1415 to +1419
/\n?export function defined\(value: any\): boolean;/gm,
`\n${readFileSync("./packages/engine/Source/Core/defined.d.ts")
.toString()
.replace(/\n*\/\*.*?\*\/\n*/gms, "")
.replace("export default", "export")}`,
Copy link
Contributor Author

@jjspace jjspace Nov 15, 2024

Choose a reason for hiding this comment

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

As per my comment when I did the same conversion into a .d.ts file for Check, I would love if we could skip this replace and pass the types file into tsd-jsdoc but we cannot. I think defined and Check are some of the only types we really need to override to take advantage of stuff like type predicates but if we add more we should definitely revisit this setup as it will quickly become too unwieldy.

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.

1 participant