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

TypeScript fixes again #10292

Merged
merged 13 commits into from
Apr 28, 2022
Merged

TypeScript fixes again #10292

merged 13 commits into from
Apr 28, 2022

Conversation

thw0rted
Copy link
Contributor

This is an updated branch for #8903. I hate to lose the conversation from there but submitting these changes as a single commit allowed me to significantly reduce the number of files in the patchset -- a lot of them were only related to when.js Promises, and that's no longer relevant. Hopefully this makes it easier to review and merge.

@cesium-concierge
Copy link

Thanks for the pull request @thw0rted!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@thw0rted thw0rted mentioned this pull request Apr 13, 2022
@thw0rted
Copy link
Contributor Author

I just noticed that #10265 was called out in the changelog -- turns out that two of the "fixes" I made to the return type info should have been correct and it was the code that was wrong. It might be worth looking at the function bodies for the other return-type changes I have in this PR to double check that there aren't other similar errors waiting to bite somebody.

@ggetz ggetz self-assigned this Apr 13, 2022
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thanks @thw0rted! I have a few comments. It appears that most of the typescript-specific question have been discussed back in #8903, so mine comments are mostly about the documentation itself.

Source/Core/HeightmapTerrainData.js Outdated Show resolved Hide resolved
Source/Scene/SingleTileImageryProvider.js Outdated Show resolved Hide resolved
Source/Core/buildModuleUrl.js Outdated Show resolved Hide resolved
Source/DataSources/CallbackProperty.js Show resolved Hide resolved
Source/DataSources/CompositePositionProperty.js Outdated Show resolved Hide resolved
Source/Core/ScreenSpaceEventHandler.js Outdated Show resolved Hide resolved
Source/Scene/ImageryProvider.js Outdated Show resolved Hide resolved
Back out SSEH setInputAction overload
Globe#material can be undefined
@ggetz
Copy link
Contributor

ggetz commented Apr 25, 2022

Thanks @thw0rted! After the last commit, the build-ts task is failing in CI. Could you please take a look?

Fixed a few tsd-jsdoc warnings
@ggetz
Copy link
Contributor

ggetz commented Apr 26, 2022

Thanks @thw0rted! Latest changes look good. Can you back out the Documentation/ changes since they're no longer needed? Then this is ready to be merged. 👍

@thw0rted
Copy link
Contributor Author

Re the Documentation changes (and the corresponding JSDoc template update), does this mean that the team isn't willing to allow for any function overloading? I think I had this conversation with Matt on the previous PR, several years ago, but at the time I hadn't figured out a way to avoid having multiple entries in the actual docs -- which is of course pretty confusing.

5+ overloads in one file looks bad, but the previous case was just one, I think so that exportKML gave the correct return type based on the options with which it was called. That would be a lot less verbose/confusing with the new system. I guess in the worst case I can always dig up the old commit if we need it again, but it'd be nice to leave it in place as an option. Your call?

@ggetz
Copy link
Contributor

ggetz commented Apr 27, 2022

I would say hold off on adding the guide and the tooling until we have a verified use case in the code. It may be the path we end up going down eventually, and we'll have the record of it in this PR. But until we have a use case in the code itself, maintaining the documentation and tooling is questionable.

@ggetz
Copy link
Contributor

ggetz commented Apr 28, 2022

Awesome, thanks @thw0rted! Thanks for your patience throughout the process.

@ggetz ggetz merged commit 3f2e627 into CesiumGS:main Apr 28, 2022
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