custom eslint plugin that fixes typedef imports #11949
Conversation
without the `yarn`, eslint could not resolve
Deploying agoric-sdk with
|
| Latest commit: |
0395b45
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://97b94ebd.agoric-sdk.pages.dev |
| Branch Preview URL: | https://ta-eslint-typedef-imports.agoric-sdk.pages.dev |
There was a problem hiding this comment.
LGTM with the following caveats, so I'll leave it to @kriskowal to approve. @kriskowal , feel free to approve after reviewing only my caveats since I really do approve the rest
There's one reason to disable the rule, which is documented and demonstrated
- I saw one file disabling the rule, which puzzled me and which I commented on. Where do I find the reason to disable the rule? Is it somewhere others are likely to find?
- I don't know enough about the internals of eslint to confidently review the code of
no-typedef-import.js. I am confident I understand what this code is trying to achieve, which LGTM. I'm just not yet confident in the implementation. (Btw, I sure wish long JS regexps were easier to read and review, but I do not suggest any changes to your code because of this.)
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable @agoric/no-typedef-import -- it's the only way to re-export with JSDoc */ | |||
There was a problem hiding this comment.
Skimming, mostly I see imports without typedefs and typedefs without imports. I do some combinations like
* @typedef { import('@agoric/swingset-liveslots').VatDeliveryObject } VatDeliveryObject
* @typedef { import('@agoric/swingset-liveslots').VatDeliveryResult } VatDeliveryResult
* @typedef { import('@agoric/swingset-liveslots').VatSyscallObject } VatSyscallObject
* @typedef { import('@agoric/swingset-liveslots').VatSyscallResult } VatSyscallResultBut why do we want to preserve that form rather than applying your fix? AFAICT, your fix would work, be meaning preserving, and be as much of an improvement here as it is where you do apply it.
There was a problem hiding this comment.
"it's the only way to re-export with JSDoc"
Could you explain this? Why do these lines uniquely suffer from this problem?
These lines are here because other modules import the above types from this module.
In .ts files we'd just use export but there's nothing like that for JSDoc because this issue is open,
In practice I think it's fine to have this rule and expect that type rollups in .js files will reduce over time.
There was a problem hiding this comment.
Thanks for the explanation. No objections.
| "clean": "rm -rf xsnap-native/xsnap/build", | ||
| "lint": "yarn run -T run-s --continue-on-error 'lint:*'", | ||
| "lint:js": "eslint 'src/**/*.js' 'test/**/*.js' api.js", | ||
| "lint:eslint": "yarn run -T eslint 'src/**/*.js' 'test/**/*.js' api.js", |
There was a problem hiding this comment.
Why the change from lint:js to lint:eslint? I don't mind the change, but I am curious.
There was a problem hiding this comment.
the :eslint is for consistency with other packages. the yarn run -T is so the command can be run from CLI (rather tha only by run-s 'lint:*')
|
Ah. To answer my own question, I do see "it's the only way to re-export with JSDoc" Could you explain this? Why do these lines uniquely suffer from this problem? |
| if (node.name.includes('$')) { | ||
| context.report({ | ||
| node, | ||
| message: 'Avoid using $ in identifiers', |
There was a problem hiding this comment.
Is the dollar-sign avoidance an orthogonal improvement, or are these somehow coupled?
There was a problem hiding this comment.
it was part of the scaffold. a subsequent commit removed it
Curious: why? With this PR, all the hard and time consuming work is already behind you, and the readability benefit is huge. Separately, any reason we should not lift this and apply directly to the endo repo? |
I just meant that it didn't have an issue and wasn't in any schedule. I intend to merge it once approved.
I don't object to someone doing that. |
unplanned
Description
#10665 has been around a while and I thought it would be good to land a custom eslint-plugin so we could add rules more easily.
This implements a simple rule to fix type imports that are done using
@typedefbecause those create new types, breaking the type nav and documentation available on hover.Security Considerations
none
Scaling Considerations
none
Documentation Considerations
There's one reason to disable the rule, which is documented and demonstrated.
Testing Considerations
CI
Upgrade Considerations
n/a