-
Notifications
You must be signed in to change notification settings - Fork 5
NEW: @W-17540434@: Implement stylelint engine's describeRules method without workspace awareness #276
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
base: dev
Are you sure you want to change the base?
NEW: @W-17540434@: Implement stylelint engine's describeRules method without workspace awareness #276
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,9 @@ | |
"types": "dist/index.d.ts", | ||
"dependencies": { | ||
"@types/node": "^20.0.0", | ||
"@salesforce/code-analyzer-engine-api": "0.23.0" | ||
"@salesforce/code-analyzer-engine-api": "0.23.0", | ||
"@types/stylelint": "^13.13.3", | ||
"stylelint": "^16.18.0" | ||
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}, | ||
"devDependencies": { | ||
"@eslint/js": "^8.57.1", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,20 @@ | ||
import { DescribeOptions, Engine, EngineRunResults, LogLevel, RuleDescription, RunOptions, Violation } from "@salesforce/code-analyzer-engine-api"; | ||
import * as fsp from 'node:fs/promises'; | ||
import { | ||
COMMON_TAGS, | ||
DescribeOptions, | ||
Engine, | ||
EngineRunResults, | ||
LogLevel, | ||
RuleDescription, | ||
RunOptions, | ||
SeverityLevel, | ||
Violation, | ||
} from "@salesforce/code-analyzer-engine-api"; | ||
import * as fsp from "node:fs/promises"; | ||
import path from "path"; | ||
import { getMessage } from "./messages"; | ||
import stylelint, { RuleMeta } from "stylelint"; | ||
import { RULE_MAPPINGS } from "./rule-mappings"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When importing from 3pp libraries like this, I recommend doing:
this way, on line 61 you should be doing
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. |
||
import { StylelintRuleStatus } from "./enums"; | ||
|
||
export class StylelintEngine extends Engine { | ||
static readonly NAME = "stylelint"; | ||
|
@@ -16,44 +29,58 @@ export class StylelintEngine extends Engine { | |
} | ||
|
||
public async getEngineVersion(): Promise<string> { | ||
const pathToPackageJson: string = path.join(__dirname, '..', 'package.json'); | ||
const packageJson: {version: string} = JSON.parse(await fsp.readFile(pathToPackageJson, 'utf-8')); | ||
const pathToPackageJson: string = path.join( | ||
__dirname, | ||
"..", | ||
"package.json" | ||
); | ||
const packageJson: { version: string } = JSON.parse( | ||
await fsp.readFile(pathToPackageJson, "utf-8") | ||
); | ||
return packageJson.version; | ||
Comment on lines
+32
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
// *** Remove underscore for private naming convention if you need any of the DescribeOptions | ||
async describeRules(_describeOptions: DescribeOptions): Promise<RuleDescription[]> { | ||
async describeRules( | ||
_describeOptions: DescribeOptions | ||
): Promise<RuleDescription[]> { | ||
// *** Best Practice - Use RunRulesProgressEvents to keep users informed on the scan progress | ||
Comment on lines
+44
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto - no changes here. Maybe your editor is applying style rules automatically during development - making unnecessary noise. |
||
this.emitRunRulesProgressEvent(0); | ||
|
||
// *** Parse out relevant file types if you engine is language-specific | ||
//const relevantFiles: string[] | undefined; | ||
|
||
Comment on lines
50
to
51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// *** Get your rules! | ||
const ruleDescriptions: RuleDescription[] = []; | ||
// Get all available rules from stylelint | ||
const allRules = stylelint.rules; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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:
|
||
this.emitRunRulesProgressEvent(50); | ||
|
||
// *** Retrieval Implementation is up to you, but you'll need to map them into RuleDescription form, such as: | ||
const exampleRule: RuleDescription = { | ||
name: "ExampleStylelintRule", | ||
severityLevel: 5, | ||
tags: [ | ||
"Recommended" | ||
], | ||
description: "The description of your rule", | ||
resourceUrls: [] | ||
}; | ||
ruleDescriptions.push(exampleRule); | ||
for (const [ruleName] of Object.entries(allRules)) { | ||
const rule = await allRules[ruleName as keyof typeof allRules]; | ||
const ruleMetadata: RuleMeta | undefined = rule.meta; | ||
if (ruleMetadata) { | ||
// do not include rules that don't have metadata | ||
|
||
ruleDescriptions.push( | ||
toRuleDescription(ruleName, ruleMetadata) | ||
); //no rules are turned on by default. | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No rules are turned on by default. I am not sure where this info will be used but it is better to be informed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
// *** Best Practice - Set RunRulesProgressEvents to 100 before completing | ||
this.emitDescribeRulesProgressEvent(100); | ||
return ruleDescriptions; | ||
|
||
return ruleDescriptions.sort((d1, d2) => | ||
d1.name.localeCompare(d2.name) | ||
); | ||
} | ||
|
||
// *** ruleNames comes from a describeRules call made just before running | ||
async runRules(_ruleNames: string[], _runOptions: RunOptions): Promise<EngineRunResults> { | ||
|
||
async runRules( | ||
_ruleNames: string[], | ||
_runOptions: RunOptions | ||
): Promise<EngineRunResults> { | ||
// *** Best Practice - Use RunRulesProgressEvents to keep users informed on the scan progress | ||
this.emitRunRulesProgressEvent(2); | ||
|
||
|
@@ -65,12 +92,77 @@ export class StylelintEngine extends Engine { | |
|
||
// *** Best Practice - Use messages sparingly to keep users informed | ||
// *** Ideal for failures, or an announcement | ||
const one = '1' | ||
this.emitLogEvent(LogLevel.Debug, getMessage('TemplateMessage2', one, '2')); | ||
const one = "1"; | ||
this.emitLogEvent( | ||
LogLevel.Debug, | ||
getMessage("TemplateMessage2", one, "2") | ||
); | ||
|
||
this.emitRunRulesProgressEvent(100); | ||
return { | ||
violations: violations | ||
violations: violations, | ||
}; | ||
} | ||
} | ||
} | ||
|
||
function toRuleDescription( | ||
ruleName: string, | ||
metadata: RuleMeta | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RuleMeta only consists of url, fixable and deprecated. We lack info on description and status. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of the rules in stylelint: alpha-value-notation. Taking this example:
|
||
): RuleDescription { | ||
let severityLevel: SeverityLevel; | ||
let tags: string[]; | ||
let status: StylelintRuleStatus = StylelintRuleStatus.NULL; | ||
|
||
if (ruleName in RULE_MAPPINGS) { | ||
status = RULE_MAPPINGS[ruleName].status ?? status; | ||
severityLevel = status | ||
? toSeverityLevel(status) | ||
: RULE_MAPPINGS[ruleName].severity; | ||
tags = RULE_MAPPINGS[ruleName].tags; | ||
} else { | ||
// Any rule we don't know about from our RULE_MAPPINGS must be a custom rule. Unit tests prevent otherwise. | ||
severityLevel = toSeverityLevel(status); | ||
tags = [...toTags(status), COMMON_TAGS.CUSTOM]; | ||
} | ||
const ruleUrl: string | undefined = metadata.url; | ||
|
||
return { | ||
name: ruleName, | ||
severityLevel: severityLevel, | ||
tags: tags, | ||
description: ruleName, //stylelint meta doesn't have a description. It can be passed only when we have a config. | ||
resourceUrls: ruleUrl ? [ruleUrl] : [], | ||
}; | ||
} | ||
|
||
export function toSeverityLevel( | ||
status: StylelintRuleStatus | undefined | ||
): SeverityLevel { | ||
if (status === StylelintRuleStatus.WARN) { | ||
// An Stylelint "warn" status is what users typically use to inform them of things without labeling it as a | ||
// violation to stop their build over. Our Info level severity is the closest to this. | ||
return SeverityLevel.Info; | ||
} else if (status === StylelintRuleStatus.ERROR) { | ||
// The "error" status is typically something users care most about, so we mark these as High severity. | ||
return SeverityLevel.High; | ||
} else if (status === StylelintRuleStatus.NULL) { | ||
// The "null" status are for marking the rules 'off', so we mark these as Low severity. | ||
return SeverityLevel.Low; | ||
} | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The severity of all rules by default is error, which is supposed to be high? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
export function toTags(status: StylelintRuleStatus | undefined): string[] { | ||
const tags: string[] = []; | ||
if ( | ||
status === StylelintRuleStatus.ERROR || | ||
status === StylelintRuleStatus.WARN | ||
) { | ||
// Any rule that base config or the user's config has turned on, should be marked as 'Recommended' | ||
// so that code analyzer will run these rules by default just as stylelint runs these rules. | ||
|
||
tags.push(COMMON_TAGS.RECOMMENDED); | ||
} | ||
return tags; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* Enum that maps to the Stylelint Rule Severity levels. | ||
* See https://stylelint.io/user-guide/configure#severity | ||
* Using the term "Status" here to differentiate from our own "Severity" term. | ||
* Also using numbers so that we can more easily compare them with > sign. | ||
*/ | ||
export enum StylelintRuleStatus { | ||
ERROR = 2, | ||
WARN = 1, | ||
NULL = 0, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).