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

docs: include FailTag jsdoc on Assert type #1393

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 6, 2022

draft of how to solve #1392

Copying the jsdoc as done in this PR makes it appear in the IDE as desired:
Screenshot 2022-12-06 at 9 48 12 AM

@SMotaal
Copy link
Member

SMotaal commented Dec 16, 2022

@turadg from past experiences with TypeScript I know there are serious drawback to the expansive use of @typedef and @type which I see here. My take is that the pattern used here will not yield the desired outcomes.

Questions

  • Is the intent of this PR to ensure that the documentation body is retained for a consumer facing (await import(…)).assert.Fail as defined in packages/ses/src/error/types.js#L277-L333?
  • Do we really need all the type coercion within the makeAssert function itself when ultimately from the consumer's viewpoint is that makeAssert is coerced to have the type MakeAssert which returns Assert?

My understanding of how the IDE works through the code relative to this PR:

  1. The FailTag JSDoc type defines the tagged literal function signature along with the documentation body in packages/ses/src/error/types.js#L277-L333.
  2. The Fail wrapped tagged template function is defined within makeAssert with @type {FailTag} in packages/ses/src/error/assert.js#L378-L379
  3. The Assert JSDoc type declares a {Fail: FailTag} in packages/ses/src/error/types.js#L335-L365
  4. The assert function is defined within makeAssert using assign(baseAssert, {Fail}) with @type {Assert} in packages/ses/src/error/assert.js#L429-L442
  5. The MakeAssert JSDoc type defines the assert maker function with @returns {Assert} in packages/ses/src/error/types.js#L233-L253
  6. The makeAssert function which returns freeze(assert) is defined with @type {MakeAssert} in packages/ses/src/error/assert.js#L360-L444
  7. The assert top-level function is defined by calling makeAssert() in packages/ses/src/error/assert.js#L448-L450
  8. The consumer-facing Fail tagged template function is obtained using (await import("./packages/ses/src/error/assert.js")).assert.Fail in some arbitrary module assumed to be at the root of this PR for convenience.

Insights

  • I am convinced that the current pattern will not yield the desired outcome.
  • I can explore alternatives and let you know when I have a working example and then we can consider how we want to refactor things — just let me know if that is the direction to take and I will get on it!

To see how broken TypeScript (version 4.9.4 per my VSCode) is for our purposes all I need to do is make the following subtle correction:

/**
 * @typedef {Object} AssertPrototype
 * @property {FailTag} Fail
 * @property {MakeAssert} makeAssert
 * 
 * assert that expr is truthy, with an optional details to describe
 * the assertion. It is a tagged template literal like
 * ```js
 * assert(expr, details`....`);`
 * ```
 *
 * The literal portions of the template are assumed non-sensitive, as
 * are the `typeof` types of the substitution values. These are
 * assembled into the thrown error message. The actual contents of the
 * substitution values are assumed sensitive, to be revealed to
 * the console only. We assume only the virtual platform's owner can read
 * what is written to the console, where the owner is in a privileged
 * position over computation running on that platform.
 *
 * The optional `optDetails` can be a string for backwards compatibility
 * with the nodejs assertion library.
 *
 * @typedef { BaseAssert & AssertPrototype & {
 *   typeof: AssertTypeof,
 *   error: AssertMakeError,
 *   fail: AssertFail,
 *   equal: AssertEqual,
 *   string: AssertString,
 *   note: AssertNote,
 *   details: DetailsTag,
 *   Fail: FailTag,
 *   quote: AssertQuote,
 *   makeAssert: MakeAssert,
 * } } Assert
 */

Now you will see the documentation body when you hover only for assert.makeAssert and not for assert.Fail.

If you switch around the order of the two @property declarations in my code you will see the documentation body for assert.Fail and not `assert.makeAssert.

With more @propety declarations added you will see that TypeScript is only honouring the last @property which is no good.

So this does not give us a solution and I am merely demonstrating how TypeScript really lacks the necessary functionality for this pattern.


Conventions

I also have some concerns regarding casing and naming conventions which are outside the scope of this PR.

My observations regarding conventions:

  • Using title-casing for <callable types>: MakeAssert, Assert, FailTag … etc.
  • Using camel-casing for <callable members>: assert.makeAssert … etc.
  • Using title-casing or camel-casing for <tagged template members>: assert.Fail, assert.details … etc.

I wonder if there is place where such conventions are addressed somewhere in the repo.

@turadg
Copy link
Member Author

turadg commented Dec 16, 2022

Is the intent of this PR to ensure that the documentation body is retained for a consumer facing (await import(…)).assert.Fail as defined in packages/ses/src/error/types.js#L277-L333?

Yes, the intent is to solve #1392. Any instance of Fail should have a hoverable doc about what it does.

Do we really need all the type coercion within the makeAssert function itself when ultimately from the consumer's viewpoint is that makeAssert is coerced to have the type MakeAssert which returns Assert?

No. Of course that's out of scope of the problem at hand.

drawback to the expansive use of @typedef and @type which I see here

I assume you mean by "here" you mean the repo because this PR has none. I expect the types will work better by making less use of @typedef, but that's not a requirement.

I can explore alternatives and let you know when I have a working example and then we can consider how we want to refactor things — just let me know if that is the direction to take and I will get on it!

@SMotaal you're more than welcome to submit another PR to solve #1392. This is a draft mostly to demonstrate an approach. I haven't had time to complete it and will probably hold off until I see what you propose.

If you want to tackle bigger changes, you're also welcome to propose some, though I can't promise they'll be supported.

@SMotaal
Copy link
Member

SMotaal commented Dec 16, 2022

This is a draft mostly to demonstrate an approach. I haven't had time to complete it and will probably hold off until I see what you propose.

I plan to open PR next week, not sure how that fits with project timelines.

If you want to tackle bigger changes, you're also welcome to propose some, though I can't promise they'll be supported.

Yes, I think I will open up a separate issue regarding conventions depending on what I dig up when looking closer at the history.

I assume you mean by "here" you mean the repo because this PR has none.

Here at this point when I checkout the PR locally as I was commenting 😊

I expect the types will work better by making less use of @typedef, but that's not a requirement.

Yes that is one aspect, but also coerced @type, unless they are intentional to make people more cognizant as a requirement.

@turadg, I guess it would be helpful if you know if that is the case?

Thanks!

@turadg
Copy link
Member Author

turadg commented Dec 17, 2022

if you know if that is the case

I think originally there was a desire to keep the type annotations in separate files, but that's no longer a requirement. In my opinion, there's no need for @callback to type functions that have one implementation and those can instead be on the function definition.

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