Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify ESLint configurations #761

Draft
wants to merge 41 commits into
base: version-4
Choose a base branch
from
Draft

Simplify ESLint configurations #761

wants to merge 41 commits into from

Conversation

phanect
Copy link
Contributor

@phanect phanect commented May 10, 2024

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

@mde
Copy link
Owner

mde commented May 10, 2024

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.:

38dddf6#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
Copy link
Owner

mde commented May 10, 2024

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

@phanect
Copy link
Contributor Author

phanect commented May 11, 2024

@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
Copy link
Contributor Author

phanect commented May 13, 2024

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants