feat: Eslint JavaScript config (try 2) #3454

giseburt · 2024-03-15T01:35:47Z

Fixes maybe #3240
Fixes maybe #3126

Building on the great work of @andrestone in #2405, which appears to be abandoned (it happens), I've been using techniques I've used in other projects and inspired by Tokens in CDK to make the code work.

The goal of this PR is two-fold:

Have ESLint write the legacy and flat formats (as outlined here)
In order to do that, we provide ability to write JavaScript-formatted config files (generally) without losing abilities like escape hatches

ESLint File Format

The yaml option has been deprecated and replaced with fileFormat valued as an enum with the options: JSON, YAML, JAVASCRIPT_FLAT_ESM, JAVASCRIPT_OLD_CJS, JAVASCRIPT_FLAT_ESM, and JAVASCRIPT_FLAT_CJS.

All of these are tested and verified with new tests. The internals still use the old file format initially, and accept commands like addOverride() that are in reference to the old file format (overrides are replaced with the settings simply being a "flat" array, where each value naturally overrides the config before it).

Each of the options are converted to a flat-format equivalent. Particularly tricky parts were plugins and parsers, where they are now imported (or required) and used as javascript. The logic used in eslint for the legacy configs was emulated to determined the module names to use, and the imports are added then used. Note that imports are NOT added to the project, but that could be added later.

Also of note what that ignorePatterns is no ignores and the pattern no longer assumes **/, so we use a heuristic: if we don't find /, **, or ! (as the first character) in the patterns, we simple prepend **/.

There's more work I'd like to do here, like making auto-fixable options warn instead of error (so they don't break auto-save with format and auto-fix in VSCode, among others), and switching rules to use the new typescript-eslint module and ESLint Stylistic where many deprecated eslint rules have moved to. These will be in other PRs and issues though.

JavascriptFile class

The new interface is highlighted by this code from the tests:

  // make a dependencies object to track imports
  const dependencies = new JavascriptDependencies();

  // add a few imports
  const [jsdoc] = dependencies.addImport("jsdoc", "eslint-plugin-jsdoc");
  const [js] = dependencies.addImport("js", "@eslint/js");

  // create a files array to modify later
  const files: Array<string> = [];

  // make a data object
  const data = [
    {
      files,
      plugins: {
        jsdoc,
      },
      rules: {
        "jsdoc/require-description": "error",

        // insert a spread operator, value doesn't matter
        [`...${js}.blah`]: true,
        "jsdoc/check-values": "error",

        // insert a second spread operator, value doesn't matter
        [`...(${jsdoc}.fakeTest ? {"fakeTest": "warn"} : {})`]: true,
      },
    },
  ];

  // now make a file with that data, including any imports, but don't resolve it yet
  const unresolvedValue = new JavascriptRaw(`${dependencies}

export default ${JavascriptDataStructure.value(data)};
`);

  // modify the data
  files.push("**/*.js");

  // now resolve the code into value
  const value = unresolvedValue.resolve();

  console.log(value);

  expect(value).toEqual(*/shown below*/);

Here's the contents of value it creates, extracted so it'll get properly syntax highlighted:

import jsdoc from 'eslint-plugin-jsdoc';
import js from '@eslint/js';

export default [
  {
    files: [
      "**/*.js",
    ],
    plugins: {
      jsdoc: jsdoc,
    },
    rules: {
      "jsdoc/require-description": "error",
      ...js.blah,
      "jsdoc/check-values": "error",
      ...(jsdoc.fakeTest ? {"fakeTest": "warn"} : {}),
    },
  },
];

Escape-hatches and late binding

Also, late-binding along with addOverride and patching escape hatches still work. A brief demonstration:

const configFileName = "testFilename.mjs";

const file = new JavascriptFile(project, configFileName, {
  obj: {
    exportedValue: "value",
  },
  marker: true,
  allowComments: true,
  cjs: false,
});

At this point the output would simply be:

// ~~ Generated by projen. To modify, edit .projenrc.js and run "npx projen".
export default {
  exportedValue: "value",
};

Now lets patch that to include the fs and instead read the value from the file name the is in readFileName:

const [newValueToken] = file.dependencies.addImport("fs", "fs");
let readFileName = "default.txt";
file.addOverride(
  "exportedValue",
  JavascriptRaw.value(
    `${newValueToken}.readFileSync(${JavascriptDataStructure.value(
      () => readFileName
    )})`
  ).toString()
);

Since we used JavascriptDataStructure.value(() => readFileName) it will (1) insert the value as data instead of raw code, which in this case is a quoted string, and (2) read readFileName at the last possible moment.

So, we can still change it:

readFileName = "finalValue.txt";

Now the output will look like:

// ~~ Generated by projen. To modify, edit .projenrc.js and run "npx projen".
import fs from 'fs';
export default {
  exportedValue: fs.readFileSync("finalValue.txt"),
};

JavascriptFile features

Take data structures and format them, with indention
- Allows for easy embedding of data into structures, as most of the types of code we're going to be making are essentially configuration
Uses stringification to Tokens, and replaces the tokens later, much like CDK Tokens (but much less sophisticated)
Accept functions as values, and data structures can be modified after being provided and before resolve() is called for late-binding data
- For example, one can include an array of values in the code that aren't known yet, but will be known before synth time
Can be built upon to add new features - the (provided) JavascriptDependencies import tracking feature for example is added in just such a way
- JavascriptDependencies can make CJS require or ESM import statements
- JavascriptDependencies addImport call returns tokens that can be used in the code as strings for the imported values (js and jsDoc in the above example)
Supports making Functions (both named and arrow)
Supports all fundamental types including Dates, Objects, and Arrays (the latter serialized with indention, unless empty)
Supports inserting spread operators in objects, quoting keys in objects (as needed), and raw javascript values in most places (providing spread operators in arrays, or calling functions, for example)

It is notably missing the ability to arbitrarily insert comments.

PS: JavascriptFile could be easily expanded to make TypeScript code as well, BTW.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2024-03-15T01:43:43Z

Codecov Report

Attention: Patch coverage is 94.55727% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 96.29%. Comparing base (ad20d2c) to head (a7bbd20).
Report is 58 commits behind head on main.

Files	Patch %	Lines
src/javascript/eslint.ts	89.50%	38 Missing ⚠️
src/javascript-file.ts	96.40%	21 Missing ⚠️
src/code-token-map.ts	96.44%	8 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3454      +/-   ##
==========================================
- Coverage   96.34%   96.29%   -0.05%     
==========================================
  Files         192      195       +3     
  Lines       37696    38944    +1248     
  Branches     3524     3711     +187     
==========================================
+ Hits        36320    37503    +1183     
- Misses       1376     1441      +65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…d reusable

mrgrain · 2024-05-01T11:20:18Z

package.json

+  "typesVersions": {
+    "<=3.9": {
+      "lib/*": [
+        "lib/.types-compat/ts3.9/*",
+        "lib/.types-compat/ts3.9/*/index.d.ts"
+      ]
+    }
+  },


That means some new code is too complex for ts3.9
I'd prefer to not have that. Can you check the file and see what is incompatible?

This came in after a rebase onto new main. I am unsure where it came from. I’ll look and see what’s up.

I was wrong. I was able to diff the two output folders in lib, and determined it was the imports in code-token-map.ts - I thought I'd use type imports, but apparently that triggered this, so I remored the type part, and now jsii has stopped adding that to the file.

mrgrain · 2024-05-01T11:21:23Z

src/code-resolvable.ts

+    // let count = 0;
+    // while (!CodeToken.isResolved(value) && count++ < 10) {
+    //   value = CodeToken.resolve(value, 0, idt);
+    // }


I’ll clean this up.

In a previous version of stringify it was practical for nested tokens to not get processed, and needed reprocessed. This is no longer the case.

mrgrain · 2024-05-01T11:24:31Z

src/javascript/eslint.ts

+   *
+   * @see https://eslint.org/docs/latest/use/configure/configuration-files-new
+   */
+  JAVASCRIPT_OLD_CJS = "old-cjs",


OLD is always a bad name. It just get's outdated. Eslint seems to call this "eslintrc format".

This should also be deprecated like the others.

I believe I saw them calling this the legacy format. I’m good either way. I agree “old” is bad naming.

And that answers my unasked question about deprecation of the others. 😄

mrgrain · 2024-05-01T11:25:16Z

src/javascript/eslint.ts

+   * @deprecated ESLINT project is transitioning away from this format, use `JAVASCRIPT_FLAT` instead
+   * @see https://eslint.org/docs/latest/use/configure/configuration-files
+   */
+  JSON = "json",


prefix with ESLINTRC_JSON

mrgrain · 2024-05-01T11:25:23Z

src/javascript/eslint.ts

+   * @deprecated ESLINT project is transitioning away from this format, use `JAVASCRIPT_FLAT` instead
+   * @see https://eslint.org/docs/latest/use/configure/configuration-files
+   */
+  YAML = "yaml",


prefix with ESLINTRC_YAML

mrgrain · 2024-05-01T11:26:21Z

src/javascript/eslint.ts

+   *
+   * @see https://eslint.org/docs/latest/use/configure/configuration-files-new
+   */
+  JAVASCRIPT_FLAT_ESM = "flat-esm",


I wonder if the format type is more important:

Enums should have the same string value as the enum:

Suggested change

JAVASCRIPT_FLAT_ESM = "flat-esm",

FLAT_JAVASCRIPT_ESM = "FLAT_JAVASCRIPT_ESM",

mrgrain · 2024-05-01T11:26:56Z

src/javascript/eslint.ts

+   * This is actually **required**, but marked as optional so upstream projects can accept this interface
+   * and provide this value.


Upstream projects should solve this themselves, sorry.

mrgrain · 2024-05-01T11:27:03Z

src/javascript/eslint.ts

   */
-  readonly dirs: string[];
+  readonly dirs?: string[];


Suggested change

readonly dirs?: string[];

readonly dirs: string[];

mrgrain · 2024-05-01T11:27:51Z

src/javascript/eslint.ts

@@ -359,7 +432,7 @@ export class Eslint extends Component {
      "*.d.ts",
      "node_modules/",
      "*.generated.ts",
-      "coverage",
+      "./coverage",


not this?

Suggested change

"./coverage",

"coverage/",

mrgrain · 2024-05-01T11:29:32Z

src/javascript/eslint.ts

+          format === EslintConfigFileFormat.JAVASCRIPT_FLAT_ESM ? "mjs" : "cjs";
+        configFileName = `eslint.config.${ext}`;
+
+        this._javascript = new JavascriptFile(project, configFileName, {


Can we encapsulate this into a new EslintConfigFile Component?

mrgrain · 2024-05-01T11:31:00Z

src/javascript/eslint.ts

  /**
   * Update the task with the current list of lint patterns and file extensions
   */
  private updateTask() {
    const taskExecCommand = "eslint";
    const argsSet = new Set<string>();
    if (this._fileExtensions.size > 0) {
-      argsSet.add(`--ext ${[...this._fileExtensions].join(",")}`);
+      if (


this feels like it's in the wrong place

mrgrain · 2024-05-01T11:31:54Z

test/javascript/eslint.test.ts

+  // EslintConfigFileFormat.JSON,
+  // EslintConfigFileFormat.YAML,
+  // EslintConfigFileFormat.JAVASCRIPT_OLD_CJS,


Oops. That was me using the testing for development, and I forgot to uncomment them. These tests are terribly slow, since they build a complete project to test actual eslint running properly.

mrgrain · 2024-05-01T11:32:23Z

src/javascript-file.ts

This file is missing docblocks

Oops, I'll add that.

mrgrain · 2024-05-01T11:33:25Z

src/javascript-file.ts

+import { JsonFile, JsonFileOptions } from "./json";
+import { Project } from "./project";
+
+export class JavascriptFunction extends CodeResolvableBase {


Add visibility to methods please

mrgrain · 2024-05-01T11:35:13Z

src/javascript-file.ts

+    private readonly properties: Array<unknown>,
+    private readonly body: unknown


Why are these unknown in type?

unknown will give errors if we try to use the value without inspecting the contents first. If we use any then is disabled typescript checks altogether. In this case, the value is passed to javascriptStringify which also take an unknown value, which forced us to validate any time we use it as before using it.

I only use any as a last resort, and usually will fail a PR I'm reviewing if I see one without a lengthy explanation of why it's necessary.

That was terribly unclear, sorry.

I'll re-evaluate all of the unknowns, but some of them are actually able to be nearly any stringable type. Some of them should only be ICodeResolvable so I'll make sure I wasn't just being sloppy.

mrgrain · 2024-05-01T11:38:24Z

src/javascript-file.ts

+        throw new Error(
+          `Default import already exists for ${from}: ${this.defaultImports.get(
+            from
+          )} !== ${imports}`


Instead of hard error, could we warn and re-write the code so that it still works? Not sure if that's feasible though.

If the user only uses the retuened token and doesn't form their own, it'll work. I'll add a warning to that affect.

mrgrain · 2024-05-01T11:40:03Z

src/javascript-file.ts

+      const defaultImport = this.defaultImports.get(from);
+      const value = imports.map((i) => i.toString()).join(", ");
+      if (defaultImport && imports.length > 0) {
+        if (this.cjs) {


I wonder if this class should have two flavours: JavascriptESMDependencies and JavascriptCJSDependencies instead of inlining a lot of if statements

I'll split it to three classes, JavascriptDependenciesBase (abstract), CJSJavascriptDependencies and ESMJavascriptDependencies. All the common logic will be in the base.

mrgrain · 2024-05-01T11:40:51Z

src/javascript-file.ts

+    options: JavascriptFileOptions
+  ) {
+    super(project, filePath, { ...options, allowComments: false });
+    this.cjs = options.cjs ?? false;


Same here, instead of a boolean flag would it be nicer to have two classes?

I think it would be a lot of duplicated code for very little gain. All the logic remains the same, just eh little bits of syntax output change.

mrgrain

This is amazing! Looking forward to getting it merged.

giseburt changed the title ~~Eslint js config2~~ feat: Eslint JavaScript config (try 2) Mar 15, 2024

giseburt force-pushed the eslint-js-config2 branch from 0242628 to d24e583 Compare April 28, 2024 15:22

andrestone and others added 8 commits April 30, 2024 21:06

feat(js-config): js config files (e.g. .eslintrc.js)

6fac0fd

Add javascript-file and refactor eslint to optionally use it

33f8534

chore: add new js-style config-file capability to eslint

6adc320

Add explanation to the JS-file test

d46c35e

fix: refactor javascript-file.ts, making tokenization more generic an…

ea85c53

…d reusable

minor code cleanup

294877b

Verify eslint functionality via tests

57a8a23

Minor cleanup

ca9bea9

giseburt force-pushed the eslint-js-config2 branch from eb5dd99 to ca9bea9 Compare May 1, 2024 02:07

giseburt marked this pull request as ready for review May 1, 2024 02:07

mrgrain reviewed May 1, 2024

View reviewed changes

mrgrain requested changes May 1, 2024

View reviewed changes

giseburt added 2 commits May 11, 2024 22:44

Some more documentation and cleanup

5fd9871

Add more docs and tests

a7bbd20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Eslint JavaScript config (try 2) #3454

feat: Eslint JavaScript config (try 2) #3454

giseburt commented Mar 15, 2024 •

edited by mrgrain

codecov-commenter commented Mar 15, 2024 •

edited

mrgrain May 1, 2024

giseburt May 1, 2024

giseburt May 4, 2024

mrgrain May 1, 2024

giseburt May 1, 2024 •

edited

mrgrain May 1, 2024 •

edited

giseburt May 1, 2024

mrgrain May 1, 2024

mrgrain May 1, 2024

mrgrain May 1, 2024

mrgrain May 1, 2024 •

edited

mrgrain May 1, 2024

mrgrain May 1, 2024

mrgrain May 1, 2024

mrgrain May 1, 2024

mrgrain May 1, 2024

giseburt May 5, 2024

mrgrain May 1, 2024

giseburt May 4, 2024

mrgrain May 1, 2024

giseburt May 4, 2024

mrgrain May 1, 2024

giseburt May 4, 2024

giseburt May 5, 2024

mrgrain May 1, 2024

giseburt May 4, 2024

mrgrain May 1, 2024

giseburt May 5, 2024

mrgrain May 1, 2024

giseburt May 5, 2024

mrgrain left a comment

	JAVASCRIPT_FLAT_ESM = "flat-esm",
	FLAT_JAVASCRIPT_ESM = "FLAT_JAVASCRIPT_ESM",

		* This is actually required, but marked as optional so upstream projects can accept this interface
		* and provide this value.

		private readonly properties: Array<unknown>,
		private readonly body: unknown

feat: Eslint JavaScript config (try 2) #3454

Are you sure you want to change the base?

feat: Eslint JavaScript config (try 2) #3454

Conversation

giseburt commented Mar 15, 2024 • edited by mrgrain

ESLint File Format

JavascriptFile class

Escape-hatches and late binding

JavascriptFile features

codecov-commenter commented Mar 15, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giseburt May 1, 2024 • edited

Choose a reason for hiding this comment

mrgrain May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment

giseburt commented Mar 15, 2024 •

edited by mrgrain

codecov-commenter commented Mar 15, 2024 •

edited

giseburt May 1, 2024 •

edited

mrgrain May 1, 2024 •

edited

mrgrain May 1, 2024 •

edited