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

Two small JSDoc/TS fixes #8928

Merged
merged 2 commits into from
Jun 8, 2020
Merged

Two small JSDoc/TS fixes #8928

merged 2 commits into from
Jun 8, 2020

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jun 7, 2020

  1. EllipsoidTangentPlane.fromPoints takes an array.
  2. EntityCollection.getById and CompositeEntityCollection.getById can
    both return undefined.

Fixes #8926
Handles bullet 1 from #8927

1. `EllipsoidTangentPlane.fromPoints` takes an array.
2. `EntityCollection.getById` and `CompositeEntityCollection.getById` can
both return undefined.
@mramato mramato requested a review from OmarShehata June 7, 2020 20:18
@cesium-concierge
Copy link

Thanks for the pull request @mramato!

  • ✔️ 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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@OmarShehata
Copy link
Contributor

The doc fixes look right to me. Do you have a minimal TypeScript config you can share with me to help me test these? Like a tsconfig.json or a package.json I can run install and run on?

Also fix missing links from previous PRs
Comment on lines -11 to +16
- Improved `MaterialProperty` JSDoc and TypeScript type definitions, which were missing the ability to take primitive types in addition to Property instances in their constructor.
- Fixed `EllipsoidGeodesic` JSDoc and TypeScript type definitions which incorrectly listed `result` as required.
- Improved `MaterialProperty` JSDoc and TypeScript type definitions, which were missing the ability to take primitive types in addition to Property instances in their constructor. [#8904](https://github.com/CesiumGS/cesium/pull/8904)
- Fixed `EllipsoidGeodesic` JSDoc and TypeScript type definitions which incorrectly listed `result` as required. [#8904](https://github.com/CesiumGS/cesium/pull/8904)
- Fixed a bug with handling of PixelFormat's flipY. [#8893](https://github.com/CesiumGS/cesium/pull/8893)
- Fixed JSDoc and TypeScript type definitions for all `ImageryProvider` types, which were missing `defaultNightAlpha` and `defaultDayAlpha` properties.
- Fixed JSDoc and TypeScript type definitions for all `ImageryProvider` types, which were missing `defaultNightAlpha` and `defaultDayAlpha` properties. [#8908](https://github.com/CesiumGS/cesium/pull/8908)
- Fixed JSDoc and TypeScript type definitions for `EllipsoidTangentPlane.fromPoints`, which takes an array of `Cartesian3`, not a single instance. [#8928](https://github.com/CesiumGS/cesium/pull/8928)
- Fixed JSDoc and TypeScript type definitions for `EntityCollection.getById` and `CompositeEntityCollection.getById`, which can both return undefined. [#8928](https://github.com/CesiumGS/cesium/pull/8928)
Copy link
Contributor

Choose a reason for hiding this comment

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

These should all be moved under a new "1.70.1" patch release right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, everything currently in 1.71.0 will be part of 1.70.1, so everything can just stay here until we update the 1.71.0 header to take the release.

@mramato
Copy link
Contributor Author

mramato commented Jun 7, 2020

@OmarShehata You have 3 options for TS PRs:

  1. Once you verify the JSDoc is correct, you can trust that the rest of our infrastructure is working.
  2. You can inspect the Cesium.d.ts file and be sure the output of the modified types are correct.
  3. You can modify Specs/TypeScript/index.ts locally and run build-ts to do what you originally asked and confirm that this is working.

For most TS-related fixes, 1 is going to be good enough but totally understand that you would want to get comfortable playing with these things so let me know if you have any problems/questions with 3. (and in reality 3 is the most thorough).

@OmarShehata
Copy link
Contributor

I can see the generated types in Cesium.d.ts are correct now, so I'm happy to merge, but I had trouble with index.ts and would like to figure this out for future PR reviews. When running build-ts I get a lot of errors, and then the task fails (but the definitions file does get regenerated). Is this expected? Below is just the last few errors:

Source/Cesium.d.ts:39872:14 - error TS2649: Cannot augment module 'ProviderViewModel' with value exports because it resolves to a non-module entity.

39872 export class ProviderViewModel {
                   ~~~~~~~~~~~~~~~~~

Source/Cesium.d.ts:40843:14 - error TS2649: Cannot augment module 'Geocoder' with value exports because it resolves to a non-module entity.

40843 export class Geocoder {
                   ~~~~~~~~

Source/Cesium.d.ts:41449:14 - error TS2649: Cannot augment module 'SelectionIndicatorViewModel' with value exports because it resolves to a non-module entity.

41449 export class SelectionIndicatorViewModel {
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~

Source/Cesium.d.ts:41660:18 - error TS2649: Cannot augment module 'Viewer' with value exports because it resolves to a non-module entity.

41660 export namespace Viewer {
                       ~~~~~~


Found 310 errors.

[12:00:53] 'build-ts' errored after 19 s
[12:00:53] Error: Command failed: npx tsc -p Specs/TypeScript/tsconfig.json
    at checkExecSyncError (child_process.js:629:11)
    at Object.execSync (child_process.js:666:13)
    at createTypeScriptDefinitions (/home/oshehata/Workspace/cesium/gulpfile.cjs:1598:17)
    at /home/oshehata/Workspace/cesium/gulpfile.cjs:199:3
    at taskWrapper (/home/oshehata/Workspace/cesium/node_modules/undertaker/lib/set-task.js:13:15)
    at bound (domain.js:402:14)
    at runBound (domain.js:415:12)
    at asyncRunner (/home/oshehata/Workspace/cesium/node_modules/async-done/index.js:55:18)
    at process._tickCallback (internal/process/next_tick.js:61:11)

@OmarShehata
Copy link
Contributor

I was able to fix this issue with Matt's suggestion over Slack by running Can you run git clean -dxf ; npm install ; npm run build ; npm run build-ts. Looks like doing npm run makeZipFIle and then build-ts causes some issues, which is a separate problem from this PR.

I was able to test the changed classes in index.ts to confirm they are working.

@OmarShehata OmarShehata merged commit 8b28104 into master Jun 8, 2020
@OmarShehata OmarShehata deleted the more-ts-fixes branch June 8, 2020 19:22
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.

Wrong type definition for EllipsoidTangentPlane.fromPoints
3 participants