mde / ejs

Embedded JavaScript templates -- http://ejs.co
Apache License 2.0
7.66k stars 834 forks source link

Simplify ESLint configurations #761

Open phanect opened 2 months ago

phanect commented 2 months ago

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.

mde commented 2 months ago

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 commented 2 months ago

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

phanect commented 2 months ago

@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 commented 2 months ago

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.