-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: migrate package to TypeScript and publish types #534
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
Conversation
migrate fixer-return migrate fixer-return
migrate consistent-output
27de051
to
64429ad
Compare
5d61735
to
fb85ff6
Compare
@@ -41,20 +39,56 @@ import requireMetaType from './rules/require-meta-type.js'; | |||
import testCasePropertyOrdering from './rules/test-case-property-ordering.js'; | |||
import testCaseShorthandStrings from './rules/test-case-shorthand-strings.js'; | |||
|
|||
const require = createRequire(import.meta.url); | |||
|
|||
const packageMetadata = require("../package.json") as { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had issues with this which I fixed in qunitjs/eslint-plugin-qunit#584
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't really apply in this case, since this file's relative position to the package.json is the same in the dist folder as it is in the lib folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks like I should be able to move index.js to fix that in my project:
I believe I've addressed all of the feedback so far. Let me know if there are any other updates you think should be made. Appreciate the thorough review. |
Added a typecheck step to the CI workflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good. Just a few more fixes.
Would you also mind just running some manual sanity checks on the compiled output? E.g. try loading the plugin from dist/
.
And make sure dist/
is ignored by tools like the following PRs (issues I discovered during the build/publishing process):
- qunitjs/eslint-plugin-qunit#585 - eslint
- qunitjs/eslint-plugin-qunit#602 - test coverage
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: 'lts/*' | ||
- run: npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, I believe we should be using npm ci
in these workflows. Fine to consider it as a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just matched the other steps. Could be a follow-up that changes them all to ci?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking a bit more on this later. In order to use npm ci
, it'll require than just updating the calls in the workflow. The repo would need to start maintaining a lock file. It currently has lock files explicitly disabled, and npm ci
requires that. I personally don't think having a lock file is a bad thing. I was kind of surprised when I first realized there wasn't one. But it seemed to be a conscious choice someone made at some point.
The other consideration is that using npm ci
in the builds will result in longer build times, if that's important to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch. I now remember that's why we don't use npm ci
. Let's avoid adding it at this time. For us, it's fine to be more aggressive with dependency updates and omit a lockfile.
@@ -41,20 +39,56 @@ import requireMetaType from './rules/require-meta-type.js'; | |||
import testCasePropertyOrdering from './rules/test-case-property-ordering.js'; | |||
import testCaseShorthandStrings from './rules/test-case-shorthand-strings.js'; | |||
|
|||
const require = createRequire(import.meta.url); | |||
|
|||
const packageMetadata = require("../package.json") as { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looks like I should be able to move index.js to fix that in my project:
@@ -75,6 +81,7 @@ | |||
"npm-run-all2": "^7.0.1", | |||
"prettier": "^3.4.1", | |||
"release-it": "^17.2.0", | |||
"tsup": "^8.5.0", | |||
"typescript": "^5.8.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this or a later PR, we can upgrade this:
"typescript": "^5.8.3", | |
"typescript": "^5.9.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of @typescript-eslint
that's installed doesn't include 5.9 in the peer deps yet. I'd suggest having a separate change that updates both sets of packages. I don't mind doing that one too.
return ( | ||
( | ||
( | ||
( | ||
( | ||
( | ||
(ast.body[0] as ExpressionStatement) | ||
.expression as AssignmentExpression | ||
).right as ObjectExpression | ||
).properties[0] as Property | ||
).value as ObjectExpression | ||
).properties[0] as Property | ||
).value as ObjectExpression | ||
).properties as Property[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering what's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's constructing the AST types that the tests all assume, based on the text of the string. It was extremely tedious, using AST Explorer
and the test strings to figure it out 😅. There's not a great way to infer the types of what a string's AST will ultimately be parsed into, yet the tests all assume the structure of the string of the test case. Definite opportunity for a large refactor of how these tests operate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thank you!
* main: feat: migrate package to TypeScript and publish types (eslint-community#534) build: convert eslint-remote-tester config to typescript (eslint-community#533) refactor: remove context compat functions (eslint-community#532) build: migrate eslint.config.js to typescript (eslint-community#531)
This change migrates all source (runtime code and tests) to TypeScript, and as a result now results in type definitions being published with the package.
Because I knew this was going to be a big PR, I tried to break things up the best I could by commit. I first
git mv
all runtime files, and then in separate commits, migrated each rule. I then did the same thing for tests.The actual changes to test files, beyond the file name change, was fairly minimal. Most of them were just simple file name changes. Only a few needed some additional work (e.g. types added to utility functions). So, I feel pretty good about maintaining functionality.
I will say, the type in
utils
aren't the best, and I had to resort to adding@ts-expect-error
in a couple places, but even then, they're in a pretty decent state. I wanted to keep the flow of the functions as close to their original form as possible in this initial cut over. All of that is to say, there's opportunity to iterate on these internal types, to improve them further. Of course, those changes wouldn't be breaking and could potentially come after v7 is released.In addition to all the runtime and test file changes, I addded a build script (using
tsup
) and updated thepackage.json
with new entry point paths and to includedist
instead oflib
. I also updated the publish GH workflow to run a build beforenpm publish
.Supporting work that I landed prior to this:
eslint.config.js
to typescript so we could use the plugin directly from source: build: migrate eslint.config.js to typescript #531eslint-remote-tester
config to be typescript so we could use the plugin directly from source: build: convert eslint-remote-tester config to typescript #533Closes #310