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

Document approach to using private utils #6866

Closed
Max10240 opened this issue May 28, 2023 · 13 comments · Fixed by #7052
Closed

Document approach to using private utils #6866

Max10240 opened this issue May 28, 2023 · 13 comments · Fixed by #7052
Labels
status: wip is being worked on by someone type: documentation an improvement to the documentation

Comments

@Max10240
Copy link
Contributor

Max10240 commented May 28, 2023

What is the problem you're trying to solve?

I'm writting a custom stylelint rule in TS, as mentioned in the document:
doc link

Stylelint has utility functions that are used in existing rules and might prove useful to you, as well. Please look through those so that you know what's available. (And if you have a new function that you think might prove generally helpful, let's add it to the list!).

I want to use some utils functions to help me implement the rule, like:
example-rule.ts

import postcss from 'postcss';
import stylelint, { utils } from 'stylelint';

import eachDeclarationBlock from 'stylelint/lib/utils/eachDeclarationBlock';
import isKeyframeSelector from 'stylelint/lib/utils/isKeyframeSelector';
import isStandardSyntaxRule from 'stylelint/lib/utils/isStandardSyntaxRule';
import isStandardSyntaxSelector from 'stylelint/lib/utils/isStandardSyntaxSelector';
import { isDeclaration } from 'stylelint/lib/utils/typeGuards';
import { isBoolean, isRegExp, isString } from 'stylelint/lib/utils/validateTypes';

const rule: stylelint.Rule = (primary, secondaryOptions) => {
  return (root, result) => {
    // use utils functions bellow
    eachDeclarationBlock(root, eachDecl => {
      eachDecl(decl => {
        isDeclaration(decl);
      });
    });

    isDeclaration(root);
    isKeyframeSelector('');
    isStandardSyntaxSelector('');
    isStandardSyntaxRule(null! as postcss.Rule);
    isBoolean(null! as unknown);
    isRegExp(null! as unknown);
    isString(null! as unknown);

    utils.report(null! as stylelint.Problem);
  }
};

rule.ruleName = '';
rule.messages = null!;

But TS complained that it could not find the declaration file:

example-rule.ts:4:34 - error TS7016: Could not find a declaration file for module 'stylelint/lib/utils/eachDeclarationBlock'. 'stylelint-plugin-example/node_modules/stylelint/lib/utils/eachDeclarationBlock.js' implicitly has an 'any' type.
  If the 'stylelint' package actually exposes this module, try adding a new declaration (.d.ts) file containing `declare module 'stylelint/lib/utils/eachDeclarationBlock';`

4 import eachDeclarationBlock from 'stylelint/lib/utils/eachDeclarationBlock';

... and more error like above

What solution would you like to see?

I can write my rule like the code above without any type errors and get type hints.

for now, I do this by modifying the local stylelint repository and linking to my custom-rule repository via "npm link", and it works well.
I create a new tsconfig file in my local stylelint project to emit declaration files:
tsconfig.emit.json

{
	"extends": "./tsconfig",
	"include": ["lib/"],
	"compilerOptions": {
		"declaration": true,
		"noEmit": false,
		"emitDeclarationOnly": true,
		"noEmitOnError": true
	}
}

then, i run tsc -p tsconfig.emit.json to emit declaration files, and link stylelint to global by npm link.
After that, I switch to my custom-rule project and run npm link stylelint, everything is fine.

@Max10240
Copy link
Contributor Author

If this feature is needed, I can create a PR for it

@ybiquitous
Copy link
Member

@Max10240 Thanks for the report and the pull request. I understand your pain, but I don't think your pull request #6867 is a good solution.

My concerns:

  • how will we manage types/stylelint/index.d.ts (manually) and auto-generated *.d.ts files?
  • should we expose all utilities? Some are only for internal use.

@Max10240
Copy link
Contributor Author

Max10240 commented May 29, 2023

@Max10240 Thanks for the report and the pull request. I understand your pain, but I don't think your pull request #6867 is a good solution.

My concerns:

  • how will we manage types/stylelint/index.d.ts (manually) and auto-generated *.d.ts files?
  • should we expose all utilities? Some are only for internal use.

Yes, I thought about both when I created this PR.
For the first question, package.json > types has higher priority by default (import stylelint from 'stylelint') unless you explicitly use import stylelint from 'stylelint/lib... '.
For the second issue, I started by manually declaring the specific utils functions I needed and then made them work using something like declare module 'stylelint/lib/utils/xxx' {}. But it didn't take long for me to get fed up, generate them all and "link" them to my "custom-rule" project. I think all exports are better than none of them. Better yet, we can talk about which functions should be exported and which shouldn't. And do declaration file generation for those that need to be exported (maybe it would be better to write the declaration file manually, I can help if I have time)

@ybiquitous
Copy link
Member

At least, we can publicly expose isStandard* utilities since we has documented them:

- `isStandardSyntax*` utilities to ignore non-standard syntax

If doing so, it seems better to expose them under the public API utils:

stylelint/lib/index.js

Lines 24 to 29 in c81e63f

utils: {
report,
ruleMessages,
validateOptions,
checkAgainstRule,
},

Any thoughts?

@Max10240
Copy link
Contributor Author

I think 'typeGuards' and' eachDeclarationBlock 'would be useful as well. Maybe we can just talk about what shouldn't be exported and export the other utils functions so TS users can use them as they want. I don't think exporting a little more is particularly unacceptable. But I'm also in favor of exporting as little as necessary (just a trade-off between ease of use and closure).

@ybiquitous
Copy link
Member

Considering our maintenance difficulty, I can't entirely agree with exposing all internal utilities.

  • For typeGuards, can you copy-and-paste only what you want? It just contains tiny functions.
  • For eachDeclarationBlock, can you work around the TS errors, for example, by using @ts-ignore at a specific line?

@Max10240
Copy link
Contributor Author

I respect your idea, let's start by exporting isStandard* utilities. But I can't resist a few words🫠:

  1. typeGuards looks like guard functions in @Babel/types, or typescript eslint/ASTUtils, such as:
import { isExpressionStatement, isForOfStatement /*...*/ } from '@babel/types';
// -------------------------------
import { ASTUtils, /*...*/ } from '@typescript-eslint/utils';
ASTUtils.isIdentifier(/*...*/)

It seems not like "just for internal use".
2. for me(just for me), "@ts-ignore" in code is almost unbearable☹️. As an aside, for other users writing rules using TS, I suggest two ways to still get type protection and autocompletion:

  • Declare the type manually
// @filename types/index.d.ts
declare module 'stylelint/lib/utils/typeGuards' {
  import { Node, Root } from 'postcss';

  export function isRoot(node: Node): node is Root;
  // ...
}

// @filename: src/custom-rule.ts
import { isRoot } from 'stylelint/lib/utils/typeGuards';
import type { Node } from 'postcss';

declare const node: Node;
isRoot(node);
  • As mentioned in this PR, use TS to automatically generate the corresponding type declaration file and paste it into the body of the declare module 'stylelint/lib/utils/xxx'
declare module 'stylelint/lib/utils/typeGuards' {
  /* paste content.... */
}

@Max10240
Copy link
Contributor Author

Also let's keep the issue open and listen to other people's opinions~

@jeddy3 jeddy3 changed the title generate declaration files (especially for files under lib/utils/) Add utilities to public API Jun 24, 2023
@jeddy3
Copy link
Member

jeddy3 commented Jun 24, 2023

Also let's keep the issue open and listen to other people's opinions

Sounds good. I'll close #5669 in favour of this broader discussion issue.

@Mouvedia
Copy link
Contributor

Mouvedia commented Jul 4, 2023

related: #6258
another affected plugin: elirasza/stylelint-stylistic#12

@jeddy3
Copy link
Member

jeddy3 commented Jul 5, 2023

I agree with @ybiquitous sentiment here and in #5669 that we should limit our public API to refactor as a small team of volunteers efficiently. Keeping Stylelint maintained benefits all our users.

The utilities we currently expose make sense. For example, stylelint.utils.validateOptions generates special invalidOption warnings that our formatters expect. In contrast, the validators used can be anything the plugin author wants.

I had wondered about the is* utilities in #5669, but they are becoming less useful as people move to vanilla CSS.

I suggest we clarify in our docs that utils should be copied and not required.

Then, either for 16.0.0 or 17.0.0, we can use export fields or bundle the code to help with the clarity. (The latter may be necessary anyway if we move to static imports for rules and formatters during our move to ESM.)

@ybiquitous
Copy link
Member

I had wondered about the is* utilities, but they are becoming less useful as people move to vanilla CSS.

I suggest we clarify in our docs that utils should be copied and not required.

I agree with this @jeddy3's idea. 👍🏼

@Mouvedia
Copy link
Contributor

Mouvedia commented Jul 5, 2023

I suggest we clarify in our docs that utils should be copied and not required.

It would be more future-proof to have a whitelist—instead of a blacklist—so that the new utils will be automatically considered not exposed by default and we won't have to update that list until #6258.

either for 16.0.0 or 17.0.0

👍 for 16

@jeddy3 jeddy3 changed the title Add utilities to public API Document approach to using private utils Jul 6, 2023
@jeddy3 jeddy3 added status: wip is being worked on by someone type: documentation an improvement to the documentation and removed status: needs discussion triage needs further discussion labels Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: documentation an improvement to the documentation
Development

Successfully merging a pull request may close this issue.

4 participants