Add Experimental support of Flat Config by uhyo · Pull Request #1522 · microsoft/vscode-eslint

uhyo · 2022-09-08T14:42:03Z

This PR adds a support of ESLint's new configuration format.

This feature is enabled by setting eslint.experimentalUseFlatConfig to true.

Note that currently FlatESLint needs to be imported from a special endpoint eslint/use-at-your-own-risk. Therefore we should treat this feature as experimental.

uhyo · 2022-09-16T04:34:03Z

@dbaeumer Hi, could you take a look when you have time? Support by VSCode's ESLint extension is a a huge step towards the spread of ESLint's new config system. (I'm not an ESLint member though 😅 )

dbaeumer · 2022-09-16T08:16:56Z

@uhyo unfortunately this has to wait a little until next month. I am very busy with task I have to complete in other areas.

koshic · 2022-09-18T16:27:17Z

@dbaeumer may be someone else can review this PR and take care about release (as alpha/beta, doesn't matter)? It doesn't look so complicated, but really blocks adoption of trash-less ESLint config.

MariaSolOs · 2022-10-01T17:06:36Z

client/src/client.ts

 				synchronize: {
 					fileEvents: [
 						Workspace.createFileSystemWatcher('**/.eslintr{c.js,c.cjs,c.yaml,c.yml,c,c.json}'),
+						Workspace.createFileSystemWatcher('**/eslint.config.js'),


You will also need to add this file to this filter:

vscode-eslint/client/src/client.ts

Line 114 in e345da0

const configFileFilter: DocumentFilter = { scheme: 'file', pattern: '**/.eslintr{c.js,c.yaml,c.yml,c,c.json}' };

MariaSolOs · 2022-10-01T17:12:45Z

server/src/eslint.ts

+	// 8.21.0 <= version, experimental endpoint that supports Flat Config
+	ESLint: undefined;
+	CLIEngine: undefined;
+	FlatESLint: ESLintClassConstructor;


From the use of this type this seems to refer the exports of ESLint main's entry point. If @dbaeumer can confirm that this was the original intention, then this last object is wrong, since the FlatESLint export is still not part of the public API. Moreover, I don't see why we would need to make ESLint undefined here? As of today ESLint is still exposing that class.

MariaSolOs · 2022-10-01T17:22:14Z

server/src/eslint.ts

 		return resultPromise;
 	}

+	function getESLintManifest(workingDirectory: DirectoryItem | undefined, configType: ConfigType | undefined) {


I truly dislike having these heuristics for figuring out the configuration type to use, since they might not match how ESLint discovers the configuration file to use and then we might end up with inconsistent linting experiences when using the CLI command versus using this extension.

I'm just giving my 2¢ here, but for preserving backwards compatibility and easing the maintenance of this extension while ESLint upgrades its API, I think we should follow a similar approach as the one used with the withESLintClass setting when moving from CLIEngine to ESLint. We could have an opt-in withFlatConfig setting that when set to true (and using the right ESLint version) will import the corresponding FlatConfig and use that API, else it will default to the current behavior until ESLint replaces the ESLint class with the flat one and then that becomes the new default. I'm aware that this would mean that if someone has an eslint.config.js file then they need an extra step of toggling the withFlatConfig setting to make this extension work but with an experimental API that's still changing, I think this is a reasonable sacrifice.

MariaSolOs · 2022-10-01T17:25:20Z

server/src/eslint.ts

-		[ '.eslintrc.yaml', false ],
-		[ '.eslintrc.yml', false ]
+	const projectFolderIndicators: [string, boolean, ConfigType | undefined][] = [
+		[ 'eslint.config.js', true, 'flat-config' ],


I would find an array of typed objects easier to understand here.

MariaSolOs · 2022-10-01T17:26:02Z

server/src/eslint.ts

+		if (ESLintModule.hasFlatESLintClass(library) && useESLintClass) {
+			return new library.FlatESLint(newOptions);
+		}


Why have this if the if-block in line 1015 would already handle it?

Also it seems like you're using the withESLintClass setting in a way that doesn't align with the documented description of the setting.

uhyo · 2022-10-02T05:07:57Z

@MariaSolOs Thank you for review! I recreated this PR with the config approach. This is indeed more reasonable; the clutter in the ESLintModule type is no longer there.

I named the new config experimentalUseFlatConfig because ESLint is going to merge the Flat Config support into the regular ESLint class, so this config will finally be unneeded after the experimental period.

Hope you can recheck this.

MariaSolOs · 2022-10-02T17:00:08Z

@uhyo Thanks to you for the quick updates! Before you do any further changes though I think that @dbaeumer (being the main maintainer of this extension) should take a look, since I don't want you to spend your precious time and effort unnecessarily :)

dbaeumer · 2022-10-07T07:21:08Z

The code changes actually look good to me. However, I would like to ask to clean up the playground you added and have one small example that uses a flat config.

dbaeumer

Clean up playground as pointed out in general comment.

uhyo · 2022-10-08T10:56:32Z

@dbaeumer Thanks! I cleaned up the playground.

MariaSolOs · 2022-10-08T17:05:16Z

$shared/settings.ts

 	validate: Validate;
 	packageManager: PackageManagers;
 	useESLintClass: boolean;
+	experimentalUseFlatConfig: boolean;


To remain consistent with the useESLintClass setting, I would remove the experimental suffix here.

Sorry for confusing you, but I'd like to keep the name experimental because, unlike useESLintClass, this config will be unnecessary in the future when Flat Config is considered stable. At that time the regular ESLint class will have the Flat Config support as described here. Then this config can be removed and users can use Flat Config without additional configuration.

I understand now. Thanks for explaining!

MariaSolOs · 2022-10-08T17:07:38Z

package.json

+					"scope": "resource",
+					"type": "boolean",
+					"default": false,
+					"description": "Enable support of experimental Flat Config (aka eslint.config.js, supported by ESLint version 8.21 or later)."


As mentioned previously, I would remove the experimental here, since flat config will eventually become stable and we don't want to have separate settings for the experimental and stable APIs.

You can dismiss this comment now that you've explained why we're using the experimental name :)

MariaSolOs · 2022-10-08T17:08:23Z

package.json

+						},
+						{


Not too important, but unless you did this on purpose, you might have a formatter making changes here.

Sorry, I reverted the formatting changes.

MariaSolOs · 2022-10-08T17:09:03Z

package.json

-								}
+					"items": {
+						"properties": {
+							"severity": {


What's the change here? Did you replace tabs with spaces?

MariaSolOs · 2022-10-08T17:10:40Z

server/src/eslint.ts

+	const projectFolderIndicators: {
+		fileName: string;
+		isRoot: boolean;


Thanks for following my suggestion! This looks much cleaner 💄

MariaSolOs · 2022-10-08T17:12:27Z

server/src/eslint.ts

+			// we need to import FlatESLint from 'eslint/use-at-your-own-risk'.
+			// See: https://eslint.org/blog/2022/08/new-config-system-part-3/
+			const eslintPath = settings.experimentalUseFlatConfig ? 'eslint/use-at-your-own-risk' : 'eslint';
 			if (nodePath !== undefined) {


I would suggest first trying to check if the 'eslint' entry point exports a FlatESLint object. If it doesn't and the setting is enabled, try the experimental path. That way we won't need to update this code when flat config becomes stable.

As described in this comment the 'eslint' entry point will never export a FlatESLint object, but the name ESLint is kept when Flat Config is stablized.

When Flat Config becomes stable, what user needs to do is to disable the experimentalUseFlatConfig config.

MariaSolOs · 2022-10-08T17:13:26Z

server/src/eslint.ts

+							}
+						} else if (lib.FlatESLint === undefined) {
+							settings.validate = Validate.off;
+							connection.console.error(`The eslint library loaded from ${libraryPath} doesn\'t neither exports a FlatESLint class.`);


I think there's a typo in this error message.

Thanks, fixed 🙂
I also updated error messages so it helps users after Flat Config is stabilized.

MariaSolOs · 2022-10-08T17:14:18Z

server/src/eslint.ts

+							}
+						} else if (library.CLIEngine === undefined && library.ESLint === undefined) {
+							settings.validate = Validate.off;
+							connection.console.error(`The eslint library loaded from ${libraryPath} doesn\'t neither exports a CLIEngine nor an ESLint class. You need at least eslint@1.0.0`);


neither exports?

MariaSolOs · 2022-10-09T16:36:52Z

server/src/eslint.ts

 							settings.validate = Validate.off;
 							if (!settings.silent) {
-								connection.console.error(`Failed to load eslint library from ${libraryPath}. See output panel for more information.`);
+								connection.console.error(`Failed to load eslint library from ${libraryPath}. If you are using ESLint lower than v8.21, try upgrading ESLint. If you are using a fairly new ESLint version, try disabling 'experimentalUseFlatConfig' config. See output panel for more information.`);


Sorry for the nit, but I think a clearer message would be "If you are using ESLint v8.21 or earlier, try upgrading it. For newer versions, try disabling the 'experimentalUseFlatConfig' setting. See the output panel for more information.".

Error messages should be as clear as possible to help users encountering them :)

Other than that, everything else LGTM! Thanks a lot @uhyo for your patience and effort, I'm sure a lot of people will very much appreciate your contribution ❤️

Fixed the error message! Thank you for patiently working with me.

dbaeumer · 2022-10-21T12:42:50Z

@MariaSolOs since you guided most of that coding can you please approve the changes as well. Then I will merge it into the main branch.

MariaSolOs · 2022-10-21T20:15:13Z

@MariaSolOs since you guided most of that coding can you please approve the changes as well. Then I will merge it into the main branch.

@dbaeumer @uhyo I tested the changes locally and everything LGTM. Let's ship this! 🚀

karlhorky · 2022-10-24T12:53:21Z

Amazing, thanks for the work from everyone here! 🙌

I take it from the milestone this will be available in the 2.2.7 release? (because the current release on the marketplace is 2.2.6)

From semver I would have imagined that this would actually go into the 2.3.0 or even 3.0.0 release, but the 2.2.next milestone that this is a part of seems to indicate that it would instead be 2.2.7.

MariaSolOs · 2022-10-25T02:35:31Z

@dbaeumer is probably going to be the one handling the release, and although I do agree with @karlhorky that 2.2.7 doesn't feel like the right semver version, I would suggest including this as part of a 2.3.0 release since we want to keep 3.0.0 for the pull diagnostics change coming up with #1534 (and in this way editors that use push diagnostics will still benefit from this flat config support).

dbaeumer · 2022-10-25T05:08:19Z

2.2.next only denote the next release. Doesn't denote a concrete version number. The version number I always decide when I do the release. It will be some wired version number increase due to the fact how the market-place handles insider versions. See https://github.com/microsoft/vscode-eslint#release-notes

karlhorky · 2022-10-25T09:46:29Z

Ok thanks, I'm not sure I understand this explanation in this Release Notes docs section, so I did a PR to try to challenge my assumptions about it:

Add more version number examples #1540

jdpnielsen mentioned this pull request Sep 15, 2022

Use new eslint config format NoA-Ignite-dk/eslint-config#42

Closed

MariaSolOs reviewed Oct 1, 2022

View reviewed changes

uhyo added 5 commits October 2, 2022 12:43

feat(client): add eslint.config.js to watch target

5a2703f

add experimentalUseFlatConfig setting

cba18ee

Support loading FlatESLint

1b4f277

add flatConfig playground

1efd8fd

fix

87b97de

uhyo force-pushed the flat-config branch from e345da0 to 87b97de Compare October 2, 2022 05:03

dbaeumer requested changes Oct 7, 2022

View reviewed changes

uhyo added 2 commits October 8, 2022 19:52

clean up flatConfig playground

d5964ab

comment in

ef614e2

MariaSolOs suggested changes Oct 8, 2022

View reviewed changes

uhyo added 3 commits October 9, 2022 17:24

revert unnecessary formatting changes

f9d0478

fix type and make error message more helpful

18f706a

update error message

a756c4d

MariaSolOs reviewed Oct 9, 2022

View reviewed changes

improve error message

d2ab5cf

karlhorky mentioned this pull request Oct 18, 2022

Improve ESLint plugin deduplication upleveled/eslint-config-upleveled#110

Closed

dbaeumer approved these changes Oct 21, 2022

View reviewed changes

MariaSolOs approved these changes Oct 21, 2022

View reviewed changes

dbaeumer merged commit 5ae084e into microsoft:main Oct 24, 2022

dbaeumer added this to the 2.2.next milestone Oct 24, 2022

uhyo deleted the flat-config branch October 24, 2022 11:43

dbaeumer modified the milestones: 2.2.next, 2.4.0 Nov 10, 2022

dbaeumer added the November 2022 label Dec 5, 2022

alexey-koran mentioned this pull request Jan 11, 2023

v0.1.0 alexey-koran/react-template#19

Merged

Conversation

uhyo commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uhyo commented Sep 16, 2022

Uh oh!

dbaeumer commented Sep 16, 2022

Uh oh!

koshic commented Sep 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhyo commented Oct 2, 2022

Uh oh!

MariaSolOs commented Oct 2, 2022

Uh oh!

dbaeumer commented Oct 7, 2022

Uh oh!

dbaeumer left a comment

Choose a reason for hiding this comment

Uh oh!

uhyo commented Oct 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhyo Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhyo Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhyo Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaeumer commented Oct 21, 2022

Uh oh!

MariaSolOs commented Oct 21, 2022

Uh oh!

karlhorky commented Oct 24, 2022

Uh oh!

MariaSolOs commented Oct 25, 2022

uhyo commented Sep 8, 2022 •

edited

Loading

uhyo Oct 9, 2022 •

edited

Loading

uhyo Oct 9, 2022 •

edited

Loading

uhyo Oct 9, 2022 •

edited

Loading

karlhorky commented Oct 25, 2022 •

edited

Loading