rewrite in js except parsing that uses oxc with meriyah as fallback#41
rewrite in js except parsing that uses oxc with meriyah as fallback#41rochdev wants to merge 17 commits intonodejs:mainfrom
Conversation
jsumners-nr
left a comment
There was a problem hiding this comment.
I haven't reviewed any of the functionality at this time. Just some maintenance type suggestions.
I'd like to see jsdoc blocks on all functions, classes, and methods. This will reduce the need to follow all paths to understand what shapes parameters are meant to be and clarify intentions.
There was a problem hiding this comment.
Please omit this file.
There was a problem hiding this comment.
It was already there (which is why I also used yarn for CI). If this is not expected it should be removed outside the scope of this PR.
There was a problem hiding this comment.
I switched to npm for commands but kept the lockfile since it's already there. I will either open a follow up PR to switch back to npm lockfile, or just remove it along with all the Rust.
There was a problem hiding this comment.
I don't see the point in any lock file in this project. They will just be ignored by any package manager when the module is installed.
There was a problem hiding this comment.
The point is deterministic builds. For example, if a new version of a dependency is released which has a bug, CI on the main branch (and all branches) will start failing, blocking any work on the library. It's better to have that happen in an isolated branch, for example a daily update by Dependabot.
There was a problem hiding this comment.
I disagree. If we want "deterministic builds" then we shouldn't be using semver ranges, but instead specific version numbers for dependencies. My opinion is that semver ranges should be kept, and bugs resolved issues resolved as they are discovered due to no lock file being present. When semver ranges and lock files are used in conjunction, no one ever knows when a problem has been introduced that will break users because the module being developed is really using semver ranges, whereas the users of the module are using the ranges.
There was a problem hiding this comment.
If we want "deterministic builds" then we shouldn't be using semver ranges, but instead specific version numbers for dependencies.
Specific version numbers don't work for issues in transitive dependencies. I don't have a strong opinion either way for this project, just stating the trade-offs. For a small project like this it shouldn't matter much, but in larger projects I can say from experience issues can arise pretty much weekly and sometimes even daily. But in this case I would expect maybe a few per year so not the end of the world.
bugs resolved issues resolved as they are discovered
Agreed, which is why I mentioned a daily Dependabot. There is nothing worse than having all PRs blocked until someone gets to fixing the main branch. But again, less of a problem in a project that doesn't get dozens of PRs per day 😄
There was a problem hiding this comment.
I'll leave this thread off with: Dependabot is a solution in search of a problem. I literally ignore every notification from it.
There was a problem hiding this comment.
I probably added it without thinking much because in general most of the repositories I work on commit the lock file and most of those repositories use yarn. This can be removed!
I personally do prefer having a lock file because it removes a some of the supply chain risk of not having one at all.
.github/workflows/ci.yml
Outdated
| - name: Install | ||
| run: yarn | ||
| - name: Run tests | ||
| run: yarn test |
There was a problem hiding this comment.
Let's please use the standard package manager: npm.
There was a problem hiding this comment.
I did at first but then noticed the project uses Yarn.
There was a problem hiding this comment.
Switched back to npm although that means CI is no longer deterministic until the lockfile is also changed.
index.js
Outdated
| "use strict"; | ||
|
|
||
| module.exports = require('./lib') |
There was a problem hiding this comment.
We have inconsistencies here:
- Double quotes in this directive, but single quotes in the
lib/compiler.jsdirective - Explicit semi-colon here, but not on line 3 (or in
lib/compiler.js)
Let's solve these sort of issues by utilizing neostandard:
npm i -D eslint neostandard
npx neostandard > eslint.config.js"scripts": {
"lint": "eslint .",
"lint:fix": "eslint --fix ."
}There was a problem hiding this comment.
I was a bit surprised by the lack of a linter but I guess the project didn't have much JS so it makes sense. Will look into neostandard.
There was a problem hiding this comment.
Added neostandard and fixed all issues. I ignored the tests folder for now since it has 1000+ issues that were already present.
Rewrite in JS except parsing that uses OXC with Meriyah as fallback.
Upcoming RFC will explain the rationale
A few features were also carried over from the original iteration of the rewrite:
astQueryfield to filter AST nodes with an esquery query. Thisis mostly meant to be used when experimenting or if what needs to be queried
is not a function.