NEW: @W-17540434@: Implement stylelint engine's describeRules method without workspace awareness #276
Conversation
|
Thanks for the contribution! It looks like @kenamverma is an internal user so signing the CLA is not required. However, we need to confirm this. |
| } | ||
| // All else will give assigned High. Recall that users may override these severities if they wish. All rules are default to error severity: https://stylelint.io/user-guide/configure#severity | ||
| return SeverityLevel.High; |
There was a problem hiding this comment.
The severity of all rules by default is error, which is supposed to be high?
There was a problem hiding this comment.
|
|
||
| ruleDescriptions.push( | ||
| toRuleDescription(ruleName, ruleMetadata) | ||
| ); //no rules are turned on by default. |
There was a problem hiding this comment.
No rules are turned on by default. I am not sure where this info will be used but it is better to be informed.
Ref: https://stylelint.io/user-guide/configure#rules
There was a problem hiding this comment.
When we get to the runRules method, most likely we'll need to generate the list that the user selected.
But for adding in the "Recommended" tag or not, that is what the rule-mappings.ts file strategy is for - since we'll also need to assign default tags and severity levels for each rule anyway.
|
|
||
| // Since we don't have test files with violations, we can at least verify the structure | ||
| expect(Array.isArray(results.violations)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Tests might change acc to new requirements but for now:
| function toRuleDescription( | ||
| ruleName: string, | ||
| metadata: RuleMeta |
There was a problem hiding this comment.
RuleMeta only consists of url, fixable and deprecated. We lack info on description and status.
export type RuleMeta = {
url: string;
deprecated?: boolean;
fixable?: boolean;
};
There was a problem hiding this comment.
So we might also use the deprecated information to decide whether or not to make the rule available or not. That is, we might want to add a filter on r=> r.meta.deprecated !== true for example.
I'm glad we have the url - so that's good.
Do you know where the description message lives then? What exactly is in the messages property of the Rule? Is that just for violation messages or is one of those a rule description? Basically the rule description typically is what they use when generating their documentation page. So you might want to take a look at the stylelint repo and figure out how they are generating their documentation. I'm assuming this metadata must live somewhere.
Or does each rule have a packaged README file with a description section or something? How does it work? I wonder if we'll have to hardcode the rule descriptions for now - or figure out a way of extracting this during a build step into a consumable descriptions file.
There was a problem hiding this comment.
This is one of the rules in stylelint: alpha-value-notation. Taking this example:
- messages only contain the violation messages: https://github.com/stylelint/stylelint/blob/main/lib/rules/alpha-value-notation/index.cjs#L19-L21
- description is the first line under the ruleName: https://github.com/stylelint/stylelint/blob/main/lib/rules/alpha-value-notation/README.md. I believe they are picking up README for their doc. We always know that the first line after the ruleName in README will be the description.
| /** Rules that analyze files that have HTML code */ | ||
| HTML: "HTML", | ||
|
|
||
| /** Rules that analyze files that have JavaScript code */ | ||
| JAVASCRIPT: "JavaScript", | ||
|
|
||
| /** Rules that analyze files that have SCSS code */ | ||
| SCSS: "SCSS", |
There was a problem hiding this comment.
Since Salesforce doesn't directly support SCSS, is this something we should be providing @stephen-carter-at-sf? I know code analyzer can support non-Salesforce products, but just wondering if this fits into our purview.
There was a problem hiding this comment.
kenamverma do any of your rules target SCSS?
If not, then yeah we should not be adding SCSS as a language tag here. Even if we did want to support SCSS files... we could still keep that under the CSS language tag anyway (sort of like how aura files call under HTML language tag).
| "@salesforce/code-analyzer-engine-api": "0.23.0", | ||
| "@types/stylelint": "^13.13.3", | ||
| "stylelint": "^16.18.0" |
There was a problem hiding this comment.
Note that I checked https://gus.lightning.force.com/lightning/o/ADM_Third_Party_Software__c/list and it appears that stylelint v16 is one of the approved 3pp libraries - so this is good.
Note that it looks like 3 days ago they published a new version. So let's get this up to date - I think 16.19.1 is what we want.
| const pathToPackageJson: string = path.join( | ||
| __dirname, | ||
| "..", | ||
| "package.json" | ||
| ); | ||
| const packageJson: { version: string } = JSON.parse( | ||
| await fsp.readFile(pathToPackageJson, "utf-8") | ||
| ); |
There was a problem hiding this comment.
Let's try to avoid updating the contents of files for stylelistic reasons. It provides noise in the code review. And it looks like the style before is actually preferable.
| async describeRules( | ||
| _describeOptions: DescribeOptions | ||
| ): Promise<RuleDescription[]> { |
There was a problem hiding this comment.
ditto - no changes here. Maybe your editor is applying style rules automatically during development - making unnecessary noise.
| // *** Parse out relevant file types if you engine is language-specific | ||
| //const relevantFiles: string[] | undefined; |
There was a problem hiding this comment.
Since you are not looking at the optional workspace provided by the DescribeOptions, then please make this PR more specific saying that it that you are implementing the describeRules method without workspace awareness as a first step.
| import path from "path"; | ||
| import { getMessage } from "./messages"; | ||
| import stylelint, { RuleMeta } from "stylelint"; |
There was a problem hiding this comment.
When importing from 3pp libraries like this, I recommend doing:
import * as stylelint from "stylelint";
this way, on line 61 you should be doing
const ruleMetadata: stylelint.RuleMeta ...`
so that we don't conflict with any other possible RuleMeta classes. Otherwise when I read RuleMeta I assume it is a type from this local package or the engine api.
| const ruleDescriptions: RuleDescription[] = []; | ||
| // Get all available rules from stylelint | ||
| const allRules = stylelint.rules; |
There was a problem hiding this comment.
Avoid untyped assignments please.
In this case, we don't need to specify the exact type that stylelint is using for its rules property - but instead just specify a type associated with what we are consuming from allRules.
Because now when I look at your line 60 I don't know what rule consists of.
It looks like you are using stylelint.MetaData from rulemeta and that's it.
So for now, I recommend making the type as follows:
type StyleLintRule = {ruleName: string, meta?: stylelint.RuleMeta};
const styleLintRuleMap: Record<string, Promise<StyleLintRule>> = stylelint.rules;
This will help eslint, the compiler, and a human know why you are using an await when referencing things.
Also, you can make this code much cleaner if you switch to a filter and a map. Here is what you can basically get it down to I believe:
type StyleLintRule = {ruleName: string, meta?: stylelint.RuleMeta};
const styleLintRuleMap: Record<string, Promise<StyleLintRule>> = stylelint.rules;
const styleLintRulePromises: Promise<StyleLintRule>[] = Object.values(styleLintRuleMap);
const styleLintRules: StyleLintRule[] = await Promise.all(styleLintRulePromises);
const ruleDescriptions: RuleDescription = styleLintRules
.filter(r => r.meta !== undefined)
.map(r => toRuleDescription(r.ruleName, r.meta));
|
|
||
| ruleDescriptions.push( | ||
| toRuleDescription(ruleName, ruleMetadata) | ||
| ); //no rules are turned on by default. |
There was a problem hiding this comment.
When we get to the runRules method, most likely we'll need to generate the list that the user selected.
But for adding in the "Recommended" tag or not, that is what the rule-mappings.ts file strategy is for - since we'll also need to assign default tags and severity levels for each rule anyway.
| function toRuleDescription( | ||
| ruleName: string, | ||
| metadata: RuleMeta |
There was a problem hiding this comment.
So we might also use the deprecated information to decide whether or not to make the rule available or not. That is, we might want to add a filter on r=> r.meta.deprecated !== true for example.
I'm glad we have the url - so that's good.
Do you know where the description message lives then? What exactly is in the messages property of the Rule? Is that just for violation messages or is one of those a rule description? Basically the rule description typically is what they use when generating their documentation page. So you might want to take a look at the stylelint repo and figure out how they are generating their documentation. I'm assuming this metadata must live somewhere.
Or does each rule have a packaged README file with a description section or something? How does it work? I wonder if we'll have to hardcode the rule descriptions for now - or figure out a way of extracting this during a build step into a consumable descriptions file.
| "SCSS" | ||
| ], | ||
| "description": "annotation-no-unknown", |
There was a problem hiding this comment.
This gold file should be called something like all-rules.goldfile.json or base-rules.goldfile.json or whatever we want to call this core set.
We should have description here - not the names of the the rules obviously. We should not have SCSS tags. And we should have this entire file reflect the document that a PM should be reviewing to ensure we have the correct severity levels and tags that we want for each rule in our rule-mappings.ts file.
|
|
||
| changeWorkingDirectoryToPackageRoot(); | ||
| jest.mock("stylelint", () => ({ |
There was a problem hiding this comment.
let's not mock out stylelint at all. If you want to provide a custom rule... then do so with an actual example in the test-data directory.
| import fs from "node:fs"; | ||
| import * as os from "node:os"; | ||
| import path from "path"; | ||
| import { StylelintEngine } from "../src/engine"; | ||
| import { changeWorkingDirectoryToPackageRoot } from "./test-helpers"; | ||
| import { RULE_MAPPINGS as _RULE_MAPPINGS } from "../src/rule-mappings"; | ||
| import { StylelintRuleStatus as _StylelintRuleStatus } from "../src/enums"; | ||
| import _stylelint from "stylelint"; |
There was a problem hiding this comment.
Please update these imports so we only have the 2 styles of imports:
import {a, b, c} from "xyz"
or
import * as xyz from "xyz"
and avoid any default imports like
import abc from "xyz".
kenamverma
changed the title
Implement stylelint engine's describeRules method without workspace awareness - base rules