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

Fix inconsistent types #5580

Closed
adalinesimonian opened this issue Oct 8, 2021 · 2 comments · Fixed by #5582
Closed

Fix inconsistent types #5580

adalinesimonian opened this issue Oct 8, 2021 · 2 comments · Fixed by #5582
Labels
type: tests an improvement to testing

Comments

@adalinesimonian
Copy link
Member

adalinesimonian commented Oct 8, 2021

@types/stylelint is poorly synchronized with the stylelint package, which is why the VS Code extension uses a script to pull down the types from the Stylelint repository (which is hackish in and of itself). However, even these type definitions are structured oddly and have some inconsistencies.

Ideally, if these inconsistencies are resolved, it would be optimal to have the Stylelint package provide its own type definitions in-package using "types": "types/stylelint/index.d.ts", which entirely avoids the problem of keeping in sync with @types/stylelint and ensures consistent typings in the Stylelint repository and downstream.

Here are a few of the issues that I've encountered downstream in the VS Code extension:

test/lib/warnings-to-diagnostics/test.js:

Also in test/workspace/rule-doc/test-plugin.js

const { lint } = /** @type {import('stylelint').StylelintPublicAPI} */ (require('stylelint'));
  • Required because lint is not defined on the stylelint module. const { lint } = require('stylelint'); results in a Property 'lint' does not exist on type 'typeof import("stylelint")'.ts(2339) error. Having the private and public API being defined as types on the module instead of a namespace export is atypical.

    Changing this may mean that the same types can be consumed both by Stylelint itself and downstream, unblocking the option of exposing types in package.json.

  • StylelintPublicAPI.lint has the type Function, which means that its return value is not typed and is not subject to type-checking without manually typing its return value. But doing that would mean that if its return type changed, the change would go unnoticed by Typescript.

test/workspace/lint/stylelint.config.js:

// TODO: Workaround for bad typings upstream
/** @type {StylelintConfig & { overrides: StylelintConfig }} */
const config = {
	// ...
	overrides: [
		// ...
		{
			files: ['**/*.scss'],
			// Workaround necessary for the following line
			customSyntax: 'postcss-scss',
		},
	],
};

StylelintConfigOverride has a very limited number of properties that are allowed, even though the override implementation allows almost every property (as far as I can tell from digging through the code). In this case, customSyntax isn't allowed by the types even though it's allowed by Stylelint, hence the workaround. Otherwise the customSyntax property results in the error:

Type '{ files: string[]; customSyntax: string; }' is not assignable to type 'StylelintConfigOverride'.
  Object literal may only specify known properties, and 'customSyntax' does not exist in type 'StylelintConfigOverride'.ts(2322)

Instead of it being defined with Pick<...>, it may make more sense to define it with Omit<...> and block the properties that aren't supported. This would also mean the type wouldn't need to be manually updated anytime a new configuration property is added.

test/workspace/rule-doc/test-plugin.js:

module.exports = stylelint.createPlugin(
	ruleName,
	// TODO: Workaround for bad typings upstream
	/** @type {import('stylelint').StylelintRule} */ (
		/** @type {unknown} */ (
			(/** @type {any} */ primaryOption) => {
				return (
					/** @type {PostCSSRoot} */ postcssRoot,
					/** @type {PostCSSResult} */ postcssResult,
				) => {
					const validOptions = stylelint.utils.validateOptions(postcssResult, ruleName, {
						actual: primaryOption,
					});

					if (!validOptions) {
						return;
					}

					stylelint.utils.report({
						ruleName,
						result: postcssResult,
						message: messages.expected,
						node: postcssRoot,
						index: 5,
					});
				};
			}
		)
	),
);

This workaround is necessary because the StylelintRule type expects specific properties added to the function, even though createPlugin doesn't require these properties, whether in the implementation or in the documentation. Without this type conversion Typescript throws the error:

Argument of type '(primaryOption: any) => (postcssRoot: PostCSSRoot, postcssResult: PostCSSResult) => void' is not assignable to parameter of type 'StylelintRule<any, any>'.
  Type '(primaryOption: any) => (postcssRoot: Root, postcssResult: PostcssResult) => void' is missing the following properties from type '{ ruleName: string; messages: StylelintRuleMessages; primaryOptionArray?: boolean | undefined; }': ruleName, messagests(2345)

Perhaps these properties are indeed necessary somewhere else in the codebase that I'm unaware of. In that event, it shouldn't be the exact same type that's used in createPlugin. Otherwise, if they are not required anywhere, they should be made optional.


Let me know if I've missed any context!

@hudochenkov
Copy link
Member

Improvement to types are always welcome.

We also have an issue open for creating TypeScript definitions #4399

adalinesimonian added a commit that referenced this issue Oct 9, 2021
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

Glad to hear it! I just submitted a PR for discussion that resolves all the problems outlined in this issue: #5582.

@adalinesimonian adalinesimonian linked a pull request Oct 9, 2021 that will close this issue
@jeddy3 jeddy3 changed the title Types are inconsistent Fix inconsistent Types Oct 9, 2021
@jeddy3 jeddy3 changed the title Fix inconsistent Types Refactor to fix inconsistent types Oct 9, 2021
@jeddy3 jeddy3 added status: wip is being worked on by someone type: tests an improvement to testing and removed status: needs discussion triage needs further discussion labels Oct 9, 2021
@adalinesimonian adalinesimonian mentioned this issue Oct 10, 2021
30 tasks
@jeddy3 jeddy3 changed the title Refactor to fix inconsistent types Add TypeScript type definitions Oct 11, 2021
jeddy3 pushed a commit that referenced this issue Oct 11, 2021
* refactor: Resolve types inconsistencies, add test

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.

* docs: Document added types in changelog

* docs: fix typo, document types in migration guide
@jeddy3 jeddy3 changed the title Add TypeScript type definitions Fix inconsistent types Oct 11, 2021
@jeddy3 jeddy3 closed this as completed Oct 11, 2021
@adalinesimonian adalinesimonian removed the status: wip is being worked on by someone label Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: tests an improvement to testing
Development

Successfully merging a pull request may close this issue.

3 participants