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 type definitions for CommonJS and ESM #7377

Merged
merged 3 commits into from Dec 8, 2023
Merged

Fix type definitions for CommonJS and ESM #7377

merged 3 commits into from Dec 8, 2023

Conversation

remcohaszing
Copy link
Contributor

This fixes the type definitions for CJS by leveraging a namespace. ESM type definitions were added, which export all types from the CJS types, and exports the exported stylelint object as default.

Some tweaks were made to the package.json exports, to resolve the types correctly for both CJS and ESM.

The type test was modified to fix CJS usage. Also a test was added for ESM usage. Stylelint was using tsconfig paths, which is a commonly misunderstood / misused feature that should rarely be used. This was removed, so TypeScript resolves the types based on package exports.

Which issue, if any, is this issue related to?

Refs 7309

Is there anything in the PR that needs further explanation?

https://arethetypeswrong.github.io/?p=stylelint@16.0.0 before this PR:

The result of ATTW showing a lot of errors

After uploading the result of npm pack after this PR:

The result of ATTW showing green checkmarks


This is best reviewed with whitespace changes disabled.

This fixes the type definitions for CJS by leveraging a namespace. ESM
type definitions were added, which export all types from the CJS types,
and exports the exported stylelint object as default.

Some tweaks were made to the package.json exports, to resolve the types
correctly for both CJS and ESM.

The type test was modified to fix CJS usage. Also a test was added for
ESM usage. Stylelint was using tsconfig `paths`, which is a commonly
misunderstood / misused feature that should rarely be used. This was
removed, so TypeScript resolves the types based on package exports.
Copy link

changeset-bot bot commented Dec 8, 2023

🦋 Changeset detected

Latest commit: 745e5b4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

StylelintPostcssResult,
Utils,
Warning,
WarningOptions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately export * from './index.js' doesn’t work if the referenced module uses export =.

@ybiquitous ybiquitous mentioned this pull request Dec 8, 2023
4 tasks
@ybiquitous
Copy link
Member

@remcohaszing Thank you so much! This change looks good to me. 👍🏼

In addition, the following type test fails, but do you know the cause?

--- a/types/stylelint/type-test.ts
+++ b/types/stylelint/type-test.ts
@@ -129,7 +129,9 @@ const testRule: RuleBase = (option) => {
 (testRule as Rule).primaryOptionArray = true;
 (testRule as Rule).meta = meta;
 
-stylelint.createPlugin(ruleName, testRule as Rule);
+const plugin = stylelint.createPlugin(ruleName, testRule as Rule);
+plugin.ruleName.toLowerCase();
+plugin.rule();
 
 messages.withNarrowedParam('should work');
$ npm -s run lint:types
types/stylelint/type-test.ts:133:8 - error TS2339: Property 'ruleName' does not exist on type 'Plugin'.
  Property 'ruleName' does not exist on type '{ default?: { ruleName: string; rule: Rule<any, any>; } | undefined; }'.

133 plugin.ruleName.toLowerCase();
           ~~~~~~~~

types/stylelint/type-test.ts:134:8 - error TS2339: Property 'rule' does not exist on type 'Plugin'.
  Property 'rule' does not exist on type '{ default?: { ruleName: string; rule: Rule<any, any>; } | undefined; }'.

134 plugin.rule();
           ~~~~


Found 2 errors in the same file, starting at: types/stylelint/type-test.ts:133

@ybiquitous
Copy link
Member

{ default?: { ruleName: string; rule: Rule } } in Plugin may be unnecessary. 🤔

export type Plugin =
| { default?: { ruleName: string; rule: Rule } }
| { ruleName: string; rule: Rule };

@ybiquitous ybiquitous changed the title Fix type definitions Fix type definitions for CommonJS Dec 8, 2023
@remcohaszing
Copy link
Contributor Author

#7377 (comment) is caused by #7377 (comment). That’s unrelated to these changes. I don’t have in-depth knowledge of Stylelint, so I can’t tell if the union with the default property is necessary.

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
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.

Got it. Thank you for the quick fix! 👍🏼

@ybiquitous ybiquitous merged commit 78deb85 into stylelint:main Dec 8, 2023
16 checks passed
@remcohaszing
Copy link
Contributor Author

Note that this didn’t just fix the types for CJS, but also for ESM.

@ybiquitous ybiquitous changed the title Fix type definitions for CommonJS Fix type definitions for CommonJS and ESM Dec 8, 2023
@ybiquitous
Copy link
Member

Thanks, I'll update the changelog before releasing it.

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.

None yet

2 participants