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

Types should be defined in .ts files #10796

Closed
2 of 6 tasks
maxpatiiuk opened this issue Nov 19, 2024 · 3 comments
Closed
2 of 6 tasks

Types should be defined in .ts files #10796

maxpatiiuk opened this issue Nov 19, 2024 · 3 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 2 Small fix or update, may require updates to tests. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone p - high Issue should be addressed in the current milestone, impacts component or core functionality

Comments

@maxpatiiuk
Copy link
Member

Check existing issues

Actual Behavior

Calcite has a number of .d.ts files that include public-facing types:
https://github.com/Esri/calcite-design-system/blob/dev/packages/calcite-components/src/components/handle/interfaces.d.ts

Usually, .d.ts files should not be authored by hand - the only case they are authored by hand is when you need to provide types for a library that is missing them - and such types would be used for internal type checking only.

To that end, tsc by default does not copy the hand-authored .d.ts files to the build output. Lumina follows this behavior. It appears than in Stencil, hand-authored .d.ts files were being copied too.

This is causing errors when type-checking JS API as interfaced.d.ts files are missing from public typings.

side note: it appears that not all Calcites interfaces files are using .d.ts - some use .ts and correctly appar in the build output:
https://github.com/Esri/calcite-design-system/blob/dev/packages/calcite-components/src/components/accordion-item/interfaces.ts

Expected Behavior

All files that include public-facing types should use .ts files

Reproduction Sample

npx tsc --outDir types --emitDeclarationOnly --declaration

Reproduction Steps

Run this from packages/calcite-components

npx tsc --outDir types --emitDeclarationOnly --declaration

See that hand-authored .d.ts files don't appear in the output, but .ts files do. Lumina has the same behavior since it's using TypeScript API behind the scenes

Reproduction Version

3.0.0-next.16

Relevant Info

No response

Regression?

No response

Priority impact

impact - p1 - need for current milestone

Impact

Breaks JS API type checking:

node_modules/@esri/calcite-components/dist/components/calcite-handle/customElement.d.ts:5:43 - error TS2307: Cannot find module './interfaces' or its corresponding type declarations.

5 import { HandleChange, HandleNudge } from './interfaces';
                                            ~~~~~~~~~~~~~~
node_modules/@esri/calcite-components/dist/components/calcite-input-time-zone/customElement.d.ts:11:43 - error TS2307: Cannot find module './interfaces' or its corresponding type declarations.

11 import { OffsetStyle, TimeZoneMode } from './interfaces';
                                             ~~~~~~~~~~~~~~
node_modules/@esri/calcite-components/dist/components/calcite-shell-panel/customElement.d.ts:4:29 - error TS2307: Cannot find module './interfaces' or its corresponding type declarations.

4 import { DisplayMode } from './interfaces';
...

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/calcite-ui-icons
  • @esri/eslint-plugin-calcite-components

Esri team

ArcGIS Maps SDK for JavaScript

@maxpatiiuk maxpatiiuk added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Nov 19, 2024
@github-actions github-actions bot added ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. calcite-components Issues specific to the @esri/calcite-components package. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone labels Nov 19, 2024
@maxpatiiuk
Copy link
Member Author

side note: we could include some sort of warning for this in Lumina.
However, there would likely be many false positives since there are valid use cases for hand-authoring .d.ts files - for providing internal typings when consuming libraries that are authored as .js only.

@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Nov 20, 2024
@jcfranco jcfranco self-assigned this Nov 20, 2024
@jcfranco jcfranco added estimate - 2 Small fix or update, may require updates to tests. and removed needs triage Planning workflow - pending design/dev review. labels Nov 20, 2024
@jcfranco jcfranco added the p - high Issue should be addressed in the current milestone, impacts component or core functionality label Nov 20, 2024
jcfranco added a commit that referenced this issue Nov 20, 2024
**Related Issue:** #10796

## Summary

Fixes TS errors caused by having public types defined in `.d.ts` files.
@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Nov 20, 2024
@github-actions github-actions bot assigned geospatialem and DitwanP and unassigned jcfranco Nov 20, 2024
Copy link
Contributor

Installed and assigned for verification.

@DitwanP
Copy link
Contributor

DitwanP commented Nov 21, 2024

🍡 Verified

@DitwanP DitwanP closed this as completed Nov 21, 2024
@DitwanP DitwanP added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 2 Small fix or update, may require updates to tests. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone p - high Issue should be addressed in the current milestone, impacts component or core functionality
Projects
None yet
Development

No branches or pull requests

4 participants