-
Notifications
You must be signed in to change notification settings - Fork 208
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
TypeScript 5.5 with @import tag #9149
Conversation
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.
LGTM, thanks!
Yes! |
63ab6f1
to
8186a9a
Compare
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 changes are small enough that it seems unlikely to be harmful to try it
the benefit is clear. oh my!
I'm curious about the typescript release cadence, but I think I won't gate my approval on researching it.
* @typedef {import('@agoric/swingset-liveslots').VatDeliveryResult} VatDeliveryResult | ||
* @typedef {import('@agoric/swingset-liveslots').VatSyscallObject} VatSyscallObject | ||
* @typedef {import('@agoric/swingset-liveslots').VatSyscallResult} VatSyscallResult | ||
* @import { VatDeliveryObject, VatDeliveryResult, VatSyscallObject, VatSyscallResult } from '@agoric/swingset-liveslots' |
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.
👏
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'm curious... can we do qualified imports? i.e. use swingset.VatSyscallResult
?
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.
Not sure I understand the question. We can still use paths to pick modules out of the package exports.
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.
by qualified import, I mean, for example:
/** @import * as swingset from '@agoric/swingset-liveslots' */
...
/**
* @param {swingset.VatDeliveryResult} result
*/
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 yes, it does. I've updated the example to work like in your comment
8186a9a
to
6e1a104
Compare
6e1a104
to
99ff07d
Compare
refs: #9149 ## Description Replace more of the `@typedef` "import" with actual `@import`. Should help with docs generation. ### Security Considerations <!-- Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? --> ### Scaling Considerations <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations <!-- Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? --> ### Upgrade Considerations <!-- What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? -->
refs: #9149 ## Description https://devblogs.microsoft.com/typescript/announcing-typescript-5-5-beta/ ### Security Considerations none ### Scaling Considerations none <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations none <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations CI <!-- Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? --> ### Upgrade Considerations none <!-- What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? -->
no ticket
Description
Adopts the 5.5 nightly to get the
@import
tag. T release plan says there will be a beta release on Apr 16. Reviewers, please evaluate whether it's worth using this pinned nightly until then.Includes a patch for gajus/eslint-plugin-jsdoc#1218
Also demonstrates a few instances of adopting the new syntax. I propose we get the capability into trunk first and separately adopt it. I think I could mostly automate it with a regex but I'd want to make that a short-lived PR to avoid merge conflicts.
Security Considerations
n/a, build only
Scaling Considerations
n/a, build only
Documentation Considerations
Once adopted we'll need to update our TypeScript style guide.
Testing Considerations
CI and also local dev. E.g. IDEs must be configured to use the workspace TypeScript version instead of what's built in. I've tested with VS Code. Maybe @Chris-Hibbert can test with JetBrains.
Upgrade Considerations
Should not affect runtime.