ejs icon indicating copy to clipboard operation
ejs copied to clipboard

Simplify ESLint configurations

Open phanect opened this issue 1 year ago • 4 comments

WIP: This PR depends on #760. I will rebase this branch after #760 is merged. Currently, the commits in this PR are a bit unreadable because it includes commits in #760, so please start reviewing after I make this PR ready to be reviewed.

  • Merge three eslint configs (eslint.config_*.mjs) to a single config file (eslint.config.js)
  • Fully migrate to ESM except for jakefile.cjs
    • Because it makes ESLint configs a bit less complex.
  • Migrate to ESLint Stylistic from deprecated built-in stylistic rules
  • Extract the lint process from Jakefile to the npm script
  • Other minor refactoring

phanect avatar May 10 '24 10:05 phanect

This is amazing! Some notes:

Use of require

I need to pull the branch to check fully, but there are probably still lingering instances of require throughout the tests, e.g.:

https://github.com/mde/ejs/pull/761/commits/38dddf660f770430e0668c9257e3d38916a054c7#diff-18aff90313b014fc8148e052d4a3602eae9b81ae10c49d6f301fa61b14bffdbdL485

Also in some of the /examples/ js files used in the test process. Unfortunately we were using require as a generic mechanism for loading and running JS code. (Jake does this as well, unfortunately.)

Testing the compiled code

I was planning on attempting to take a split approach to testing, so we we could run the same set of tests on the ESM code, and also directly on the compiled CJS code. (Something similar to what I was doing in the ESLint.)

Right now, 100% of people using EJS are using the CJS code, and this new approach puts a lot of trust in the ability of tsc to generate completely equivalent code for them. We are at least doing a major semver release bump, I would feel a lot better releasing this if I knew that the CJS code would continue to be 100% compliant with all tests, even if we are making significant changes to the source code.

mde avatar May 10 '24 22:05 mde

Also adding you as a contributor to make this easier. これからもよろしくお願いいたします!

mde avatar May 10 '24 22:05 mde

@mde Thank you for the review! Because I was working another task today, I will check your comment and update this PR later.

Also adding you as a contributor to make this easier.

Thank you, こちらこそよろしくお願いします!

phanect avatar May 11 '24 09:05 phanect

Sorry for the delay. I've been scatterbrained in recent weeks and working on different projects day by day... :sweat_smile:

Use of require

I need to pull the branch to check fully, but there are probably still lingering instances of require throughout the tests, e.g.:

38dddf6#diff-18aff90313b014fc8148e052d4a3602eae9b81ae10c49d6f301fa61b14bffdbdL485

The link didn't work unfortunately, but did you mean this?

var d = require('domain').create();

Yes, I forgot to replace inline requires. They can be replaced with dynamic import() unless they are used in synchronous functions.

Testing the compiled code

I was planning on attempting to take a split approach to testing, so we we could run the same set of tests on the ESM code, and also directly on the compiled CJS code. (Something similar to what I was doing in the ESLint.)

Right now, 100% of people using EJS are using the CJS code, and this new approach puts a lot of trust in the ability of tsc to generate completely equivalent code for them. We are at least doing a major semver release bump, I would feel a lot better releasing this if I knew that the CJS code would continue to be 100% compliant with all tests, even if we are making significant changes to the source code.

That makes sense. I will revert the test code to CommonJS and consider if we can use a single eslint.config.js without entirely migrating to ESM.

phanect avatar May 13 '24 22:05 phanect