-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat(ses): Expand TypeScript coverage for Compartment and lockdown #584
Conversation
I'll review this when tests pass. Feel free to ping me when ready. |
8962de7
to
95406c8
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.
Looks good! Just minor suggestions.
packages/ses/src/error/assert.js
Outdated
// without breaking the meaning of the type. As currently written, if it is | ||
// moved into a separate @callback type, it no longer understands that `args` | ||
// is a rest parameter. I have not yet figured out how to declare that it is, | ||
// except by having it here directly annotating the `details` function. |
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 have mixed feelings about @callback
.
I'd recommend moving it to types.js
and declaring it as something like:
/** @typedef {(template: TemplateStringsArray, ...args: Array<any>) => ...} DetailsTag */
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, this is diff churn. @erights added the DetailsTag
type to types.js
already and this is a holdover from before I rebased.
I’m removing this text. Though, for posterity, I think that @callback
is useful in JSDoc over what appears in types.js
because it produces an opportunity to describe each parameter with prose to go with the type and name. I look forward to using jsdoc for docs too!
// @ts-check | ||
|
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.
Duplicate.
// @ts-check |
This change includes Compartment and lockdown in the SES shim’s exported types and generally extends the reach of TypeScript verification into SES sources. This change uses the expedient of reexporting the return types of functions that are checked by
yarn tsc:lint
which may not be sufficient. In a subsequent change, it will likely be necessary to convert the Compartment and lockdown types into interface definitions that are imported by the sources and also exported byindex.d.ts
.