Conversation
|
Thank you for the pull request, @donmccurdy! ✅ We can confirm we have a CLA on file for you. |
| const pattern = process.argv[2]; | ||
|
|
||
| if (!pattern) { | ||
| console.error(`Usage: node ./scripts/lebab-batch.js "path/to/*.js"`); | ||
| process.exit(2); | ||
| } |
There was a problem hiding this comment.
Small drive-by comment. It might not be required given the simplicity and small scale of this script but we already use yargs regularly in our repo for CLI argument processing. May make this more robust especially if you want to add options later
scripts/lebab-batch.js
Outdated
|
|
||
| console.log(bright(`Converting...\n`)); | ||
|
|
||
| for await (const path of glob(resolve(__dirname, "..", pattern))) { |
There was a problem hiding this comment.
We also tend to use globby for file paths. I didn't actually even realize node:fs provided a builtin glob function. I'm not sure if there are behavioral differences but just a suggestion.
There was a problem hiding this comment.
@jjspace I haven't checked, but maybe, maybe 🤞 there are "behavioral differences" in that the built-in, node-based one works better with Windows paths? (You remember the back and forth around #12631 (comment) that was caused by globby not accepting actual paths...)
There was a problem hiding this comment.
Update after looking at the underlying issue again: There is a comment at sindresorhus/globby#155 (comment) pointing to https://github.com/sindresorhus/globby?tab=readme-ov-file#convertpathtopatternpath 💡
There was a problem hiding this comment.
The Node.js glob() function is fairly new, introduced in Node.js v22 — before there were a lot of different custom solutions for promises vs. callbacks, CJS vs. ESM, etc. I'd encourage us to consider moving toward the Node.js built-in function over time. Similarly, I think our dependency on urijs could be replaced with the built-in URL class nowadays.
There was a problem hiding this comment.
I still run Node 20 so I actually couldn't run this file in that case (I hadn't actually tried it yet)
I agree with moving toward built-ins as much as possible but as a library we support Node versions >=20 as 20 is still in LTS. Maybe there's motivation to change that? or maybe it doesn't matter as much in this case and we can require Node 22 for maintainers that want to run this script?
There was a problem hiding this comment.
Node v20 is in maintenance mode now and reaches end-of-life in two months so it would probably be good for us to start updating local and CI environments. I don't mind switching the script to use globby though, to be consistent.
|
Also might be good to add a comment about how to use and run this script to the section of the Docs you added about the type conversion |
|
I've updated the script to use |
Description
Checking in a batch script to help with ES6 class conversion. The script, and the
lebabdevDependency, can be deleted after migration is completed.Usage:
node ./scripts/lebab-batch.js "path/to/SomePattern*.js"As mentioned in #13209, I ran into an issue recently because lebab does not automatically split JSDoc comments into class-level and constructor-level comments (it's all the same comment with prototype-based inheritance) so I've added a check in the script to catch this and warn the developer to manually fix these comments. Output of the check would be:
Issue number and link
Testing plan
Developer-facing script, used for earlier PRs linked in #8359.
Author checklist
CONTRIBUTORS.mdI have updatedCHANGES.mdwith a short summary of my changeI have added or updated unit tests to ensure consistent code coverageI have updated the inline documentation, and included code examples where relevantPR Dependency Tree
This tree was auto-generated by Charcoal