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

Help refactor #174

Merged
merged 29 commits into from
Jun 28, 2024
Merged

Help refactor #174

merged 29 commits into from
Jun 28, 2024

Conversation

lishaduck
Copy link
Contributor

Reland the remaining changes from #126.

Depends on #168.
Here's the diff as GH doesn't play well with stacks from forks: lishaduck/node-elm-review@eslints...help-refactor

lishaduck and others added 3 commits June 24, 2024 17:26
Make Jest run less often.
Before now, Jest would run on every change.

Also, ESLint now fails on warnings.
See (jfmengels#163)[jfmengels#163 (comment)]

Co-authored-by: Jeroen Engels <jfm.engels@gmail.com>
@lishaduck lishaduck mentioned this pull request Jun 25, 2024
23 tasks
@lishaduck lishaduck marked this pull request as draft June 25, 2024 17:35
@lishaduck

This comment was marked as outdated.

@lishaduck lishaduck force-pushed the help-refactor branch 2 times, most recently from f9983cd to a45324f Compare June 25, 2024 17:44
@lishaduck lishaduck marked this pull request as ready for review June 25, 2024 17:44
@lishaduck
Copy link
Contributor Author

Ok, I decided to approach this differently then #126 did. It doesn't always return true, it can also never return and exit instead. I've update the types to reflect this.

@lishaduck lishaduck mentioned this pull request Jun 25, 2024
Copy link
Owner

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

lishaduck added 17 commits June 26, 2024 22:45
`eslint-plugin-n` is the maintained version of `eslint-plugin-node`.
Now that we only support Node 14.18 and later, we can use `node:`.
I just realized that I'd reverted the bump to Node 14.18.
As I've gotten stuff dependent on 14.18 merged, I'm bumping it again.
Oops. At least it's only a single minor.

This also allows `node:` to stay.
XO removed it a while back, but it now supports ESLint 8.
This replaces `util.promisify` with `fs.promises` for all asynchronous operations.
This also switches from `node:fs` to `graceful-fs` for all synchronous operations.
When possible, we now use async/await rather than callbacks.
While a slight performance hit is likely incurred, the code is much more consistent.
More Promise-based apis are available now than used to be the case.
Replace some jarring .then() chains with `async`/`await`.
Well, technically, it's @eslint-community/eslint-plugin-eslint-comments.
Same difference. And hey, it caught something!
I hadn't previously realized that they didn't drop Node 16 until v7.
The plugin requires a newer version of Node to run on ESLint v8.
Also, I ran `npm pkg fix`. It didn't find anything.

In conclusion, I sorted the package.json file via `npx sort-package-json`.
This makes try/catch more predictable.
I figured this was a hangover.
prettier/prettier is already removed.
Unicorn renamed these rules a long time ago.
`*` is an "explicit any!"
Continue making sure the code works everwhere.
Maybe ESM someday, but for now...
Turbo kills them otherwise, which is often fine, but breaks `test-run`.
Our custom error message handling was overly decentralized.
@jfmengels jfmengels merged commit 06e7df8 into jfmengels:main Jun 28, 2024
3 checks passed
@jfmengels
Copy link
Owner

Thank you for your work on this @lishaduck!

@lishaduck lishaduck deleted the help-refactor branch June 28, 2024 19:15
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.

2 participants