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

Add TypeScript type definitions #5582

Merged
merged 4 commits into from Oct 11, 2021
Merged

Add TypeScript type definitions #5582

merged 4 commits into from Oct 11, 2021

Conversation

adalinesimonian
Copy link
Member

@adalinesimonian adalinesimonian commented Oct 9, 2021

Closes #4399, Fixes #5580

  • Types are refactored to be exportable and are now provided in the package:

    • Stylelint prefix removed where appropriate, types namespaced under stylelint.

    • Module export definition changed to point to namespace and public API, which allows require('stylelint') to be typed correctly.

    • Types renamed to match those in the @types/stylelint package to reduce incompatibilities/breakages downstream.

    • As a result of these changes, the separate types package can be deprecated in favour of the built-in type definitions.

  • Internal API endpoints marked with @internal where appropriate.

  • PostCSS types in types definition file scoped under PostCSS (e.g. PostCSS.Root) to make it clearer which types are imported (example).

  • API types (stylelint.lint, stylelint.createPlugin, etc.) changed from Function to more specific types that actually reflect each function's signature.

  • Refactored export types in public-facing exports (e.g. lib/index.js, lib/createPlugin.js) to reference API types in type definition file. This ensures that Typescript will report errors if the implementation/internal type annotations are changed to an incompatible signature. Any types that were only defined in those files as @typedef JSDoc comments have been moved to the .d.ts definition file (for example, the Violation type).

  • Fixed problem with Rule types that resulted in Typescript errors when using createPlugin. Required properties were added in Improve types for at-rule-* rules #5418, which broke usage of createPlugin as the rule parameter would require functions to have the newly added properties, even though this doesn't reflect the implementation accurately. To fix this:

  • Fixed Typescript errors being thrown when the configs passed to the overrides option would contain supported properties that were missing from the types definition:

    • ConfigOverride type updated to use Omit instead of Pick, removing the overrides property instead of allow-listing properties. This fix means the ConfigOverride type shouldn't need to be manually updated whenever a new configuration property is added.
  • RuleMessages and related types fixed to avoid Typescript errors. The return type of stylelint.utils.ruleMessages was incorrectly set to the type of the messages parameter, which resulted in type errors in lib/ruleMessages.js. This change:

    • Changes the ruleMessages return type to { [K in keyof T]: T[K] }, which is correct. There is a distinction between returning the same type as being passed to the function and returning a type with the same keys and value types for those keys.

    • RuleMessages type changed to a mapped type to facilitate the change to stylelint.utils.ruleMessages. This allows Typescript to narrow types for the object returned by ruleMessages:

      const messages = stylelint.utils.ruleMessages(ruleName, {
        violation: 'This a rule violation message',
        warning: (reason: string) => `This is not allowed because ${reason}`,
      });
      // If a `Record<...>` was used instead, the following lines would
      // result in Typescript error 2322.
      const violationMessage: string = messages.violation;
      const violationFunc: Function = messages.warning;
    • Type annotations in lib/utils/ruleMessages.js updated to refect these changes and to avoid Typescript errors and allow type-checking.

  • Type test file added to types/stylelint/type-test.ts. This file contains code that references the public API as a consuming package would use it. It is not executed, but is used so that if the API or types are changed in a breaking way, the change will be caught by Typescript. If this existed before, the breaking change to the StylelintRule type (now stylelint.Rule) would have been caught.

  • Documentation comments added to public API endpoints (stylelint.*).

  • Removed leftover duplicate Options type that had no references anywhere in the project.

Fixes #5580

- Types are refactored to be exportable and are now provided in the
  package:

  - `Stylelint` prefix removed where appropriate, types namespaced under
    `stylelint`.

  - Module export definition changed to point to namespace and public
    API, which allows `require('stylelint')` to be typed correctly.

  - Types renamed to match those in the `@types/stylelint` package to
    reduce incompatibilities/breakages downstream.

  - As a result of these changes, the separate types package can be
    deprecated in favour of the built-in type definitions.

- Internal API endpoints marked with `@internal` where appropriate.

- PostCSS types in types definition file scoped under `PostCSS` (e.g.
  `PostCSS.Root`) to make it clearer which types are imported.

- API types (`stylelint.lint`, `stylelint.createPlugin`, etc.) changed
  from `Function` to more specific types that actually reflect each
  function's signature.

- Refactored export types in public-facing exports (e.g. `lib/index.js`,
  `lib/createPlugin.js`) to reference API types in type definition file.
  This ensures that Typescript will report errors if the implementation/
  internal type annotations are changed to an incompatible signature.
  Any types that were only defined in those files as `@typedef` JSDoc
  comments have been moved to the `.d.ts` definition file.

- Fixed problem with `Rule` types that resulted in Typescript errors
  when using `createPlugin`. Required properties were added in #5418,
  which broke usage of `createPlugin` as the `rule` parameter would
  require functions to have the newly added properties, even though this
  doesn't reflect the implementation accurately. To fix this:

  - `Rule` type split into `RuleBase` and `Rule`. Added the `Plugin`
    type as an alias for `RuleBase` to achieve parity with former types
    at `@types/stylelint`. `RuleBase` represents the rule without any
    properties attached. `Rule` is a union of `RuleBase` and an object
    with the properties.

  - `createPlugin` updated to use `RuleBase`.

- Fixed Typescript errors being thrown when the configs passed to the
  `overrides` option would contain supported properties that were
  missing from the types definition:

  - `ConfigOverride` type updated to use `Omit` instead of `Pick`,
    removing the `overrides` property instead of allow-listing
    properties. This fix means the `ConfigOverride` type shouldn't need
    to be manually updated whenever a new configuration property is
    added.

- `RuleMessages` and related types fixed to avoid Typescript errors. The
  return type of `stylelint.utils.ruleMessages` was incorrectly set to
  the type of the `messages` parameter, which resulted in type errors in
  `lib/ruleMessages.js`. This change:

  - Changes the `ruleMessages` return type to `{ [K in keyof T]: T[K] }`
    which is correct. There is a distinction between returning the same
    type as being passed to the function and returning a type with the
    same keys and value types for those keys.

  - `RuleMessages` type changed to a mapped type to facilitate the
    change to `stylelint.utils.ruleMessages`. This allows Typescript to
    narrow types for the object returned by `ruleMessages`:

    ```js
    const messages = stylelint.utils.ruleMessages(ruleName, {
      violation: 'This a rule violation message',
      warning: (reason: string) => `This is not allowed because ${reason}`,
    });
    // If a `Record<...>` was used instead, the following lines would
    // result in Typescript error 2322.
    const violationMessage: string = messages.violation;
    const violationFunc: Function = messages.warning;
    ```

  - Type annotations in `lib/ruleMessages.js` updated to refect these
    changes and to avoid Typescript errors and allow type-checking.

- Type test file added to `types/stylelint/type-test.ts`. This file
  contains code that references the public API as a consuming package
  would use it. It is not executed, but is used so that if the API or
  types are changed in a breaking way, the change will be caught by
  Typescript. If this existed before, the breaking change to the
  `StylelintRule` type (now `stylelint.Rule`) would have been caught.

- Documentation comments added to public API endpoints (`stylelint.*`).

- Removed leftover duplicate `Options` type that had no references
  anywhere in the project.
@adalinesimonian
Copy link
Member Author

Updated PR comment with links to each relevant change in the code, where appropriate.

@adalinesimonian adalinesimonian linked an issue Oct 9, 2021 that may be closed by this pull request
@adalinesimonian adalinesimonian changed the title refactor: Resolve types inconsistencies, export types, add type test refactor: Resolve type inconsistencies, export types, add type test Oct 9, 2021
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Terrific!

types/stylelint/index.d.ts Show resolved Hide resolved
@adalinesimonian adalinesimonian mentioned this pull request Oct 10, 2021
30 tasks
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@adalinesimonian Fantastic! Thank you so much for your work! 👏🏼

This change will expose Stylelint's type definition, so I suggest:

@adalinesimonian adalinesimonian changed the title refactor: Resolve type inconsistencies, export types, add type test feat: Export types Oct 11, 2021
@adalinesimonian adalinesimonian linked an issue Oct 11, 2021 that may be closed by this pull request
@adalinesimonian
Copy link
Member Author

@ybiquitous I've renamed the PR, linked #4399 to it, and added a note to the changelog in aa98de1

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Amazing work, thank you!

Suggested a minor typo change.

As per #5205 (comment), should we add a note to the migration guide? Something at the bottom along the lines of:


Integration authors

TypeScript type definitions are now exported in the stylelint package. If you use the @types/stylelint package, you should remove it from your dependencies.

CHANGELOG.md Outdated Show resolved Hide resolved
@jeddy3 jeddy3 changed the title feat: Export types Add TypeScript type definitions Oct 11, 2021
@adalinesimonian
Copy link
Member Author

@jeddy3 Fixed the typo and added a note about types to the migration guide.

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!

LGTM.

@jeddy3 jeddy3 merged commit 816b19a into v14 Oct 11, 2021
@jeddy3 jeddy3 deleted the v14-refactor-types branch October 11, 2021 15:35
adalinesimonian added a commit to stylelint/vscode-stylelint that referenced this pull request Oct 11, 2021
adalinesimonian added a commit to stylelint/vscode-stylelint that referenced this pull request Oct 11, 2021
* refactor: use upstream types directly w/o script

Made possible by stylelint/stylelint#5582

* fix: avoid typescript error, conform to new types

Disables are no longer reported directly on results and are now reported
just like other rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix inconsistent types Create TypeScript definitions
4 participants