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

Updates to typedefs and documentation defaults #12193

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Conversation

lukemckinstry
Copy link
Contributor

@lukemckinstry lukemckinstry commented Sep 13, 2024

Description

Type definition and documentation fix for the problem identified & described here #11749 (comment)
which identifies an inconsistency in the documentation where we list default: undefined but our typedef requires a specific type.

Made fixes using the following logic

  • if the argument is required in pragmas on the constructor
    • make sure it is not marked optional in the constructor args
    • make sure the @default is not listed as undefined (either change to default used in constructor or remove the line entirely)
  • if the argument is optional
    • change the typedef to allow undefined eg ... | undefined

This PR covers fixes for Source/Core, other areas of the codebase will be covered in future PRs

Issue number and link

#11749 (comment)

Testing plan

  • build the documentation npm run build-docs and note changes to default values for targeted properties
  • when using cesium with typescript, note you can set targeted properties to undefined

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

github-actions bot commented Sep 13, 2024

Thank you for the pull request, @lukemckinstry! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

@ggetz
Copy link
Contributor

ggetz commented Sep 13, 2024

Thanks @lukemckinstry! I can confirm we now have you on file in the Cesium CLA.

@lukemckinstry
Copy link
Contributor Author

I am reconsidering one section of this PR concerning the 4 Frustum classes (....Frustum.js) covered here.
These classes all have optional parameters on the constructor (there are no pragmas in the constructor requiring these attributes). For these optional attributes in the PR I changes their types to be ... | undefined, unless they specify a default value. So for attributes with default values they cannot later be reassigned to undefined, only attributes without defaults can.
Does this make sense? Would it be useful to design the types so the developer could later reassign those attributes to undefined as well?

@ggetz
Copy link
Contributor

ggetz commented Sep 16, 2024

So for attributes with default values they cannot later be reassigned to undefined, only attributes without defaults can.
Does this make sense? Would it be useful to design the types so the developer could later reassign those attributes to undefined as well?

Let me confirm my understanding– You're suggesting that we should allow the properties with default value to be set to undefined, correct?

@ggetz
Copy link
Contributor

ggetz commented Sep 16, 2024

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

Usually we don't include documentation changes in CHANGES.md. However, since this affects the behavior when using typescript, we should be sure to add a bullet for these fixes.

@lukemckinstry lukemckinstry changed the title Updates to typedefs and documentation defaults in Source/Core Updates to typedefs and documentation defaults Sep 16, 2024
@lukemckinstry
Copy link
Contributor Author

Let me confirm my understanding– You're suggesting that we should allow the properties with default value to be set to undefined, correct?

as discussed, this additional change is not necessary as the primary purpose of this PR is to update type definitions for optional parameters to include undefined.

@lukemckinstry
Copy link
Contributor Author

@ggetz updated CHANGES.md so this is ready for review (pending whether the CI occurs again)

@ggetz
Copy link
Contributor

ggetz commented Sep 17, 2024

Awesome, thanks @lukemckinstry!

@ggetz ggetz merged commit 8b4a16b into main Sep 17, 2024
9 checks passed
@ggetz ggetz deleted the ts-types-allow-undefined branch September 17, 2024 12:51
@javagl
Copy link
Contributor

javagl commented Sep 28, 2024

Just stumbled over this: I assume that the first checkbox from #11749 (comment) can now be ticked. (At least, I did that, updating that comment to point to this PR)

@javagl javagl mentioned this pull request Sep 28, 2024
5 tasks
@lukemckinstry
Copy link
Contributor Author

Just stumbled over this: I assume that the first checkbox from #11749 (comment) can now be ticked. (At least, I did that, updating that comment to point to this PR)

Hey @javagl, this didn't address all instances of the problem, only most of the ones that occurred in Source/Core, so I think the comment should be left unchecked until we address all cases.

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.

3 participants