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

refactor(types): rewrite component TypeScript definitions #385

Merged
merged 12 commits into from
Nov 19, 2020

Conversation

metonym
Copy link
Collaborator

@metonym metonym commented Nov 3, 2020

This PR rewrites the TypeScript definitions from scratch.

Purpose

  • account for intrinsic element attributes if $$restProps is spread to a native HTML Element (Fix IntrinsicAttributes error in TypeScript definitions #304 )
  • accurately represent the shape of the library so direct imports work
  • improve typings for prop default values by deferring to the Svelte compiler instead of JSDocs
  • improve typings for forwarded/dispatched events
  • improve typings for slots, slot props

Refactor

  • adjust existing JSDoc annotations to be more concise
  • remove "." from component file names to improve direct imports
  • update typeof import("carbon-icons-svelte/lib/Add16").default to import("carbon-icons-svelte").CarbonIcon
  • remove type annotation for id, ref props where applicable

Docs

  • replace PUBLIC_API.json with COMPONENT_API.json generated from "sveld"
  • enhance component API documentation by linking to component source code, labelling reactive props, describing if the component spreads $$restProps

Todo

  • further improve slot prop typings (conservatively, some will fallback to any)
  • some interfaces should be exported for the consumer (i.e. DataTable headers in DataTable: typings for headers and rows #364 ) (defer)
  • use svelte-check to validate TS definitions in the CI

Errors

  • props must extend svelte.JSX.HTMLAttributes<HTMLElementTagNameMap["div"]>; an intersection will cause errors
  • components without $$restProps (or if it's ambiguous like Tag) should conservatively allow any attributes
  • remove "undefined" dispatched events

@vercel
Copy link

vercel bot commented Nov 3, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-svelte/carbon-components-svelte/iu056uky3
✅ Preview: https://carbon-components-svelte-git-rewrite-tyepscript-definitions.carbon-svelte.vercel.app

@albertms10
Copy link
Contributor

albertms10 commented Nov 4, 2020

Wow, nice job! 👏

Just asking… Will union types also be rewritten to TS syntax? (As do the *.d.ts output type files)

Before:

/**
 * Specify alignment of accordion item chevron icon
 * @type {"start" | "end"}
 */
export let align = "end";

After:

/** Specify alignment of accordion item chevron icon */
export let align: "start" | "end" = "end";

@metonym
Copy link
Collaborator Author

metonym commented Nov 4, 2020

@albertms10 That's good thinking but unfortunately that would make the uncompiled Svelte components TS only.

export let hasIconOnly = false;

/**
* Specify the icon from `carbon-icons-svelte` to render
* @type {typeof import("carbon-icons-svelte/lib/Add16").default} [icon]
* @type {typeof import("carbon-icons-svelte/lib/Add16").default}
Copy link
Contributor

@albertms10 albertms10 Nov 5, 2020

Choose a reason for hiding this comment

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

Instead of typing the generic icon prop with the Add16 icon default type, wouldn’t it be cleaner to use the typeof CarbonIcon class?—it applies to the rest of icon imports.

/**
 * Specify the icon from `carbon-icons-svelte` to render
 * @type {typeof import("carbon-icons-svelte").CarbonIcon}
 */
export let icon = undefined;

Although in this way you can’t restrict the icon size to e.g. 16… 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice suggestion. The icon size should not matter.

@metonym metonym changed the title refactor(types): rewrite type definitions to account for intrinsic attributes refactor(types): rewrite component TypeScript definitions Nov 6, 2020
Replaces PUBLIC_API.json with COMPONENT_API.json generated by sveld.
@metonym metonym marked this pull request as ready for review November 19, 2020 22:28
@metonym metonym merged commit 81593b6 into master Nov 19, 2020
@metonym metonym deleted the rewrite-tyepscript-definitions branch November 19, 2020 22:29
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.

None yet

2 participants