Repository: stylelint/vscode-stylelint (ts)

Issue:
Show old Stylelint warning regardless of if document is configured to be linted
As mentioned in https://github.com/stylelint/vscode-stylelint/issues/284#issuecomment-972638636, in workspaces with an old copy of Stylelint, the error will not show unless the open document is configured to be linted in the extension using `stylelint.validate`. However, this isn't great because if the user had an environment in which they were using non-standard syntax and relying on the automatic inferral of syntaxes, they won't get a notification with a link to the migration guide when opening a non-standard file. So effectively, the extension doesn't lint and is silent about the old copy of Stylelint.

We should change the warning's conditions such that it will still warn the user regardless of if the file that's been opened is configured to be linted.

Pull request description:
feat: Warn for old Stylelint regardless of language ID
Closes #333 

Does not warn if Stylelint was resolved globally, only locally.

Patch:
diff --git a/src/server/modules/old-stylelint-warning.ts b/src/server/modules/old-stylelint-warning.ts
index a2cce63..ba436ac 100644
--- a/src/server/modules/old-stylelint-warning.ts
+++ b/src/server/modules/old-stylelint-warning.ts
@@ -1,6 +1,7 @@
 import path from 'path';
 import fs from 'fs/promises';
 import semver from 'semver';
+import pathIsInside from 'path-is-inside';
 import type { TextDocument } from 'vscode-languageserver-textdocument';
 import type LSP from 'vscode-languageserver-protocol';
 import { getWorkspaceFolder } from '../../utils/documents';
@@ -54,7 +55,10 @@ export class OldStylelintWarningModule implements LanguageServerModule {
 		this.#openMigrationGuide = capabilities.window?.showDocument?.support ?? false;
 	}
 
-	async #getStylelintVersion(document: TextDocument): Promise<string | undefined> {
+	async #getStylelintVersion(
+		document: TextDocument,
+		workspaceFolder: string,
+	): Promise<string | undefined> {
 		const result = await this.#context.resolveStylelint(document);
 
 		if (!result) {
@@ -75,6 +79,14 @@ export class OldStylelintWarningModule implements LanguageServerModule {
 			return undefined;
 		}
 
+		if (!pathIsInside(packageDir, workspaceFolder)) {
+			this.#logger?.debug('Stylelint package root is not inside the workspace', {
+				uri: document.uri,
+			});
+
+			return undefined;
+		}
+
 		const manifestPath = path.join(packageDir, 'package.json');
 
 		try {
@@ -95,17 +107,6 @@ export class OldStylelintWarningModule implements LanguageServerModule {
 	}
 
 	async #check(document: TextDocument): Promise<string | undefined> {
-		const options = await this.#context.getOptions(document.uri);
-
-		if (!options.validate.includes(document.languageId)) {
-			this.#logger?.debug('Document should not be validated, ignoring', {
-				uri: document.uri,
-				language: document.languageId,
-			});
-
-			return undefined;
-		}
-
 		const workspaceFolder = await getWorkspaceFolder(this.#context.connection, document);
 
 		if (!workspaceFolder) {
@@ -126,7 +127,7 @@ export class OldStylelintWarningModule implements LanguageServerModule {
 
 		this.#checkedWorkspaces.add(workspaceFolder);
 
-		const stylelintVersion = await this.#getStylelintVersion(document);
+		const stylelintVersion = await this.#getStylelintVersion(document, workspaceFolder);
 
 		if (!stylelintVersion) {
 			return undefined;


Test patch:
diff --git a/src/server/modules/__tests__/old-stylelint-warning.ts b/src/server/modules/__tests__/old-stylelint-warning.ts
index 3c87804..56ca6a9 100644
--- a/src/server/modules/__tests__/old-stylelint-warning.ts
+++ b/src/server/modules/__tests__/old-stylelint-warning.ts
@@ -19,14 +19,16 @@ const mockedGetWorkspaceFolder = getWorkspaceFolder as jest.MockedFunction<
 >;
 const mockedFindPackageRoot = findPackageRoot as jest.MockedFunction<typeof findPackageRoot>;
 
-const mockContext = serverMocks.getContext();
-const mockLogger = serverMocks.getLogger();
+let mockContext: ReturnType<typeof serverMocks.getContext>;
+let mockLogger: ReturnType<typeof serverMocks.getLogger>;
 
 describe('OldStylelintWarningModule', () => {
 	beforeEach(() => {
+		jest.clearAllMocks();
 		mockedPath.__mockPlatform('posix');
+		mockContext = serverMocks.getContext();
+		mockLogger = serverMocks.getLogger();
 		mockContext.__options.validate = [];
-		jest.clearAllMocks();
 	});
 
 	test('should be constructable', () => {
@@ -42,8 +44,9 @@ describe('OldStylelintWarningModule', () => {
 		expect(mockContext.documents.onDidOpen).toHaveBeenCalledWith(expect.any(Function));
 	});
 
-	test('if document language ID is not in options, should not warn', async () => {
-		mockContext.__options.validate = ['baz'];
+	test('if document is not part of a workspace, should not warn', async () => {
+		mockedGetWorkspaceFolder.mockResolvedValue(undefined);
+		mockContext.__options.validate = ['bar'];
 
 		const module = new OldStylelintWarningModule({
 			context: mockContext.__typed(),
@@ -63,13 +66,13 @@ describe('OldStylelintWarningModule', () => {
 		expect(mockContext.resolveStylelint).not.toHaveBeenCalled();
 		expect(mockContext.connection.window.showWarningMessage).not.toHaveBeenCalled();
 		expect(mockLogger.debug).toHaveBeenLastCalledWith(
-			'Document should not be validated, ignoring',
-			{ uri: 'foo', language: 'bar' },
+			'Document not part of a workspace, ignoring',
+			{ uri: 'foo' },
 		);
 	});
 
-	test('if document is not part of a workspace, should not warn', async () => {
-		mockedGetWorkspaceFolder.mockResolvedValue(undefined);
+	test('if document has already been checked, should not warn', async () => {
+		mockedGetWorkspaceFolder.mockResolvedValue('/path');
 		mockContext.__options.validate = ['bar'];
 
 		const module = new OldStylelintWarningModule({
@@ -81,22 +84,27 @@ describe('OldStylelintWarningModule', () => {
 
 		module.onDidRegisterHandlers();
 
+		const document = TextDocument.create('foo', 'bar', 1, 'a {}');
 		const handler = mockContext.documents.onDidOpen.mock.calls[0][0];
 
-		await handler({
-			document: TextDocument.create('foo', 'bar', 1, 'a {}'),
-		});
+		await handler({ document });
+		await handler({ document });
 
-		expect(mockContext.resolveStylelint).not.toHaveBeenCalled();
+		expect(mockContext.resolveStylelint).toHaveBeenCalledTimes(1);
 		expect(mockContext.connection.window.showWarningMessage).not.toHaveBeenCalled();
 		expect(mockLogger.debug).toHaveBeenLastCalledWith(
-			'Document not part of a workspace, ignoring',
+			'Document has already been checked, ignoring',
 			{ uri: 'foo' },
 		);
 	});
 
-	test('if document has already been checked, should not warn', async () => {
+	test('if Stylelint package root cannot be determined, should not warn', async () => {
 		mockedGetWorkspaceFolder.mockResolvedValue('/path');
+		mockedFindPackageRoot.mockResolvedValue(undefined);
+		mockContext.resolveStylelint.mockResolvedValue({
+			stylelint: {} as unknown as stylelint.PublicApi,
+			resolvedPath: '/path/node_modules/stylelint',
+		});
 		mockContext.__options.validate = ['bar'];
 
 		const module = new OldStylelintWarningModule({
@@ -108,28 +116,30 @@ describe('OldStylelintWarningModule', () => {
 
 		module.onDidRegisterHandlers();
 
-		const document = TextDocument.create('foo', 'bar', 1, 'a {}');
 		const handler = mockContext.documents.onDidOpen.mock.calls[0][0];
 
-		await handler({ document });
-		await handler({ document });
+		await handler({
+			document: TextDocument.create('foo', 'bar', 1, 'a {}'),
+		});
 
 		expect(mockContext.resolveStylelint).toHaveBeenCalledTimes(1);
 		expect(mockContext.connection.window.showWarningMessage).not.toHaveBeenCalled();
-		expect(mockLogger.debug).toHaveBeenLastCalledWith(
-			'Document has already been checked, ignoring',
-			{ uri: 'foo' },
-		);
+		expect(mockLogger.debug).toHaveBeenLastCalledWith('Stylelint package root not found', {
+			uri: 'foo',
+		});
 	});
 
-	test('if Stylelint package root cannot be determined, should not warn', async () => {
+	test('if Stylelint package root is not in workspace, should not warn', async () => {
+		const error = new Error('foo');
+
 		mockedGetWorkspaceFolder.mockResolvedValue('/path');
-		mockedFindPackageRoot.mockResolvedValue(undefined);
+		mockedFindPackageRoot.mockResolvedValue('/usr/local/lib/node_modules/stylelint');
 		mockContext.resolveStylelint.mockResolvedValue({
 			stylelint: {} as unknown as stylelint.PublicApi,
 			resolvedPath: '/path/node_modules/stylelint',
 		});
 		mockContext.__options.validate = ['bar'];
+		mockedFS.readFile.mockRejectedValue(error);
 
 		const module = new OldStylelintWarningModule({
 			context: mockContext.__typed(),
@@ -148,9 +158,10 @@ describe('OldStylelintWarningModule', () => {
 
 		expect(mockContext.resolveStylelint).toHaveBeenCalledTimes(1);
 		expect(mockContext.connection.window.showWarningMessage).not.toHaveBeenCalled();
-		expect(mockLogger.debug).toHaveBeenLastCalledWith('Stylelint package root not found', {
-			uri: 'foo',
-		});
+		expect(mockLogger.debug).toHaveBeenLastCalledWith(
+			'Stylelint package root is not inside the workspace',
+			{ uri: 'foo' },
+		);
 	});
 
 	test('if Stylelint package manifest cannot be read, should not warn', async () => {
@@ -501,6 +512,35 @@ describe('OldStylelintWarningModule', () => {
 		expect(mockLogger.warn).toHaveBeenCalledWith('Failed to open migration guide');
 	});
 
+	test('if document language ID is not in options, should warn', async () => {
+		mockedGetWorkspaceFolder.mockResolvedValue('/path');
+		mockedFindPackageRoot.mockResolvedValue('/path/node_modules/stylelint');
+		mockContext.resolveStylelint.mockResolvedValue({
+			stylelint: {} as unknown as stylelint.PublicApi,
+			resolvedPath: '/path/node_modules/stylelint',
+		});
+		mockContext.__options.validate = ['baz'];
+
+		const module = new OldStylelintWarningModule({
+			context: mockContext.__typed(),
+			logger: mockLogger,
+		});
+
+		module.onInitialize({ capabilities: {} } as unknown as LSP.InitializeParams);
+
+		module.onDidRegisterHandlers();
+
+		const handler = mockContext.documents.onDidOpen.mock.calls[0][0];
+
+		await handler({
+			document: TextDocument.create('foo', 'bar', 1, 'a {}'),
+		});
+
+		expect(mockContext.resolveStylelint).toHaveBeenCalledTimes(1);
+		expect(mockContext.connection.window.showWarningMessage).toHaveBeenCalledTimes(1);
+		expect(mockContext.connection.window.showDocument).not.toHaveBeenCalled();
+	});
+
 	it('should be disposable', () => {
 		const module = new OldStylelintWarningModule({
 			context: mockContext.__typed(),
diff --git a/test/e2e/__tests__/__snapshots__/code-actions.ts.snap b/test/e2e/__tests__/__snapshots__/code-actions.ts.snap
index 4cdf6bf..9386e0f 100644
--- a/test/e2e/__tests__/__snapshots__/code-actions.ts.snap
+++ b/test/e2e/__tests__/__snapshots__/code-actions.ts.snap
@@ -121,3 +121,12 @@ Array [
   },
 ]
 `;
+
+exports[`Code actions should run auto-fix action on save 1`] = `
+"a {
+    font-size: 1.2em;
+    /* stylelint-disable-next-line comment-no-empty */
+    color: #00;
+}
+"
+`;
diff --git a/test/e2e/__tests__/code-actions.ts b/test/e2e/__tests__/code-actions.ts
index 20f8849..96e6a78 100644
--- a/test/e2e/__tests__/code-actions.ts
+++ b/test/e2e/__tests__/code-actions.ts
@@ -1,4 +1,5 @@
 /* eslint-disable jest/no-standalone-expect */
+import fs from 'fs/promises';
 import path from 'path';
 
 import * as JSONC from 'jsonc-parser';
@@ -34,10 +35,22 @@ const jsPath = path.resolve(workspaceDir, 'code-actions/test.js');
 const settingsPath = path.resolve(workspaceDir, 'code-actions/.vscode/settings.json');
 
 const defaultSettings = {
+	'[css]': {
+		'editor.codeActionsOnSave': {
+			'source.fixAll.stylelint': true,
+		},
+	},
 	'stylelint.reportNeedlessDisables': true,
 	'stylelint.validate': ['css', 'javascript'],
 };
 
+const css = `a {
+  font-size: 1.2em;
+    /* stylelint-disable-next-line comment-no-empty */
+  color: #00;
+}
+`;
+
 // TODO: Investigate why editing tests intermittently fail on CI
 const localIt = process.env.CI ? it.skip : it;
 
@@ -49,6 +62,8 @@ describe('Code actions', () => {
 	});
 
 	afterEach(async () => {
+		await fs.writeFile(cssPath, css);
+
 		const settingsEditor = await openDocument(settingsPath);
 		const text = settingsEditor.document.getText();
 		const settings = JSONC.parse(text) as unknown;
@@ -105,6 +120,23 @@ describe('Code actions', () => {
 		expect(actions).toHaveLength(0);
 	});
 
+	test('should run auto-fix action on save', async () => {
+		const editor = await openDocument(cssPath);
+
+		await waitForDiagnostics(editor);
+
+		await getCodeActions(editor);
+
+		// API won't save unless we dirty the document, unlike saving via the UI
+		await editor.edit((editBuilder) => {
+			editBuilder.insert(new Position(3, 0), ' ');
+		});
+
+		await editor.document.save();
+
+		expect(editor.document.getText()).toMatchSnapshot();
+	});
+
 	localIt('should disable rules for an entire file', async () => {
 		const editor = await openDocument(cssPath);
 
@@ -173,7 +205,7 @@ describe('Code actions', () => {
 
 		await settingsEditor.edit((edit) =>
 			edit.insert(
-				new Position(5, 2),
+				new Position(10, 2),
 				',\n\t"stylelint.codeAction.disableRuleComment": { "location": "sameLine" }',
 			),
 		);
diff --git a/test/e2e/setup.ts b/test/e2e/setup.ts
index 721cecd..49bd141 100644
--- a/test/e2e/setup.ts
+++ b/test/e2e/setup.ts
@@ -19,7 +19,7 @@ Object.defineProperty(global, 'workspaceDir', {
 	},
 });
 
-let openEditors: TextEditor[] = [];
+const openEditors: TextEditor[] = [];
 
 Object.defineProperty(global, 'openDocument', {
 	get() {
@@ -123,7 +123,7 @@ global.afterEach(async () => {
 		await commands.executeCommand('workbench.action.files.revert');
 	}
 
-	openEditors = [];
+	openEditors.length = 0;
 
 	await commands.executeCommand('workbench.action.closeAllEditors');
 });
diff --git a/test/e2e/workspace/code-actions/.vscode/settings.json b/test/e2e/workspace/code-actions/.vscode/settings.json
index 1c808bb..caa129a 100644
--- a/test/e2e/workspace/code-actions/.vscode/settings.json
+++ b/test/e2e/workspace/code-actions/.vscode/settings.json
@@ -1,4 +1,9 @@
 {
+	"[css]": {
+		"editor.codeActionsOnSave": {
+			"source.fixAll.stylelint": true
+		}
+	},
 	"stylelint.reportNeedlessDisables": true,
 	"stylelint.validate": [
 		"css",