@aduth checkout this file, I typed everything like this and then ported it back into src/index.js. I think it is better typed, though to integrate TS files into the project needs more work. I just wanted to offer this up as another solution.

https://github.com/johnhooks/hpq/blob/172fec770524b0b10ed5a2d3e0ff1fd427740633/src/index.ts

I'm still learning about hpq so excuse my ignorance, it seems like matchers might be typed something like this, but I'm not sure. I would like to get rid of the any types.

type MatcherFn = (node: Element) => string|undefined;
type MatcherObj = Record<string, MatcherFn>;
type Matcher = MatcherFn | MatcherObj;

Does a MatcherFn always return string or undefined? Or is it possible to access non-string values on Element?

Thanks for the quick turnaround on the pull request @johnhooks !

I might not be able to take a deep dive on this until the weekend, and shamefully I have to admit that it's been a while since the last major update to this library, so I'm having to refamiliarize myself with it as well!

A couple quick comments to what you've surfaced:

• Looking at the overloading in the TypeScript version, I think I'd be open to porting this to TypeScript if it'd make it easier for you to type it
• To your question about the return type of MatcherFn, I think conceptually I had imagined it could return anything in the sense that someone could provide their own matcher function that returns a boolean, for example. However, given the set of default matchers, I think it'd be fair to say string or undefined would be the standard expected return type, if it helps avoid the any type.
  • I'd wonder if there's any way to still support someone who might want to define their own matcher which has an arbitrary return type? Maybe a generic and inferred ReturnType?
• I think you're correct that I had mistakenly mixed up ? and = in assigning the "optional" type
• To your question about output directory, I think it makes sense as you've proposed, though it's also probably time for a rethink as far as what outputs the library supports broadly speaking (i.e. standardizing on ESM)

@aduth thanks! I'm excited to help.

Looking at the overloading in the TypeScript version, I think I'd be open to porting this to TypeScript if it'd make it easier for you to type it

The library seems simple, though it is actually hard to know exactly what is being accessed when the matchers are defined. I am willing to take some time and get it right. And would be happy to help maintain a TS version.

I'd wonder if there's any way to still support someone who might want to define their own matcher which has an arbitrary return type? Maybe a generic and inferred ReturnType?

I agree, there should be a way to infer the return type. It might take a little refactoring to make it work.

To your question about output directory, I think it makes sense as you've proposed, though it's also probably time for a rethink as far as what outputs the library supports broadly speaking (i.e. standardizing on ESM)

I for one would support ESM only, that shouldn't affect the largest dependent of this package, Gutenberg. Using tsc build output for modules and types would simplify the build to a single step.

It's really cool that this little library is used deep down in the core of Gutenberg. And it's obviously doing a great job because it was able to just sit for four years without any issues!

Was looking if there might be a way to infer the return type based on the input. Definitely challenging with the overloaded signature. This seems to look okay at a glance, though unsure yet how well it could be ported back to JSDoc:

https://www.typescriptlang.org/play?#code/C4TwDgpgBAsghsAxgCwgJwGIDsA8GB8UAvFABRYD2AJhAFxQCiANhALYRbACUxhcWIANwAoUJFgIU6APIAjAFZ5CJAEoREFNFRwBnYGgCWWAOYAaCUlSZcBfCOEAzAK5ZEwAxSxQwcNDoh4UBAAHsAcVDoWUtY4-CD45tJBoeGR8JYyCkr45NR0jCzsnOasklY69NJc9ADeUADaANJQRlAA1hAgFA5Q0gC69GrATmhYACrgAdJNfYQAviLOru6e3r7+gSFhWBFRVtixAvg5lDT0zGwcwCVl6BVQGNVQQyPjk0qLLm4eXj5+ARhkttdulogc4glekDUntMopbLkzgVLsUoKUMn5KlAAD4PHg1YRQIlQNAQYajKA1BbCObCYQaLB6EkQHROJjAYhrf6kKgURBOIrAAB0smoIHMdQcFAo9EREB4REIpwgQrCoQAwp5thy5lwRAymaTWeyAEycv7+Hl8gVXEVi8ykUVUEAKwhOkBCoxYdAACTGMAAMnqgA

That looks really good to me.

From what I learned the only way to "overload" the function in JSDocs is a union type, and that would look pretty messy and require const parser = function() {} syntax to type it as a union of multiple functions types.

Do you want me to submit a PR with the other overloads I created and you commit the parse function signature? And we convert this to TypeScript ? It does seem to be a better fit.

I just discovered that the newly-released TypeScript 5.0 includes an @overload directive for JSDoc, which sounds like it might be the perfect fit here.

https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#overload-support-in-jsdoc

That sound awesome! I'll read up on it.

That just came out, I skimmed through it two days ago and didn't catch @overload, so many great improvements!

This seems to be working okay for me in testing locally, want me to go ahead and push it up? Or do you want to take a closer look at it?

/**
 * @template {(node: Element) => any} F
 * @typedef {(node: Element) => any} MatcherFn
 */

/**
 * @template {(node: Element) => any} F
 * @typedef {Record<string, MatcherFn<F>>} MatcherObj
 */

/**
 * @template {MatcherObj<F>} O
 * @template {MatcherFn<any>} F
 *
 * @overload
 * @param {string|Element} source Source content
 * @param {O} matchers Object of matchers
 * @return {{[K in keyof O]: ReturnType<O[K]>}} Matched values, shaped by object
 */

/**
 * @template {MatcherFn<any>} F
 *
 * @overload
 * @param {string|Element} source Source content
 * @param {F} matcher Matcher function
 * @return {ReturnType<F>} Matched value
 */

/**
 * Given a markup string or DOM element, creates an object aligning with the
 * shape of the matchers object, or the value returned by the matcher.
 *
 * @template {MatcherObj<F>} O
 * @template {MatcherFn<any>} F
 *
 * @param {string|Element} source Source content
 * @param {O|F} matchers Matcher function or object of matchers
 */
export function parse(source, matchers) {
  // ...
}

I just pushed something up. Sorry I can walk it back. Can I also do the comments like that rather than all spaced out like they do on Gutenberg?

@aduth I was going to walk some of it back, but it was breaking everything else. Could you take a look at the current version, and push any updates. I'll stop working on it until I hear from you.

I'm really happy with how this turned out. Thanks so much for point out @overload it was fun to take advantage of TS 5.0 so quickly.

Hey, no worries and no need to walk anything back. I had to step away for a bit anyways, so it worked out. And it looks like you've got most everything in place now. Glad that new feature worked well!

Only issue I see with the new feature is that none of the comments came with the overloads, in the output es/index.d.ts

And yes, I'd be fine to collapse the whitespace in the JSDoc if you want to do that.

Only issue I see with the new feature is that none of the comments came with the overloads, in the output es/index.d.ts

Hm, yeah, I see what you mean 🤔 Looks like it could be partly related to microsoft/TypeScript#27981 ? (Edit: Maybe not...) Not sure if there's a syntax variation to get those comments to "stick" into the output.

@aduth I tried adding the description to all the overloads, but nothing worked. Other than that, I think it's ready for review.

@aduth I tried adding the description to all the overloads, but nothing worked. Other than that, I think it's ready for review.

Yeah, I didn't have any luck either. Do you think it should be a blocker? I'm okay to ship it without the comments if you are. Seems like it might be a bug in TypeScript itself. Only other option I could conceive is to port to TypeScript.

@johnhooks I pushed up changes in 60a48ca to check-in the .d.ts file. I also created an issue on the TypeScript repository at microsoft/TypeScript#53350 in case it helps toward a resolution. Does this look okay to you?

One other thing that comes to mind is that the matcher object form supports arbitrary nesting of objects. Would it be easy enough to change the MatcherObj type from Record<string, MatcherFn<T>> to Record<string, MatcherObj<T> | MatcherFn<T>>? This is another behavior that unfortunately didn't have good test coverage 😕 , but works all the same.

Playground example:

@aduth That issue filed on TypeScript looks great to me.

That type makes sense to me though the type checker isn't happy with it.

/**
 * @typedef {(node: Element) => T} MatcherFn
 * @template T
 */

/**
 * @typedef {Record<string, MatcherObj<T> | MatcherFn<T>>} MatcherObj
 * @template T
 */

/**
 * @typedef {(MatcherFn<T>|MatcherObj<T>)} Matcher
 * @template T
 */

Results in:

Type alias 'MatcherObj' circularly references itself.ts(2456)

I have always had trouble understanding the nuance of circular references, sometimes they work fine.

This works though...

/**
 * @typedef {(node: Element) => T} MatcherFn
 * @template T
 */

/**
 * @typedef {{[ x: string ]: MatcherObj<T> | MatcherFn<T>} } MatcherObj
 * @template T
 */

/**
 * @typedef {(MatcherFn<T>|MatcherObj<T>)} Matcher
 * @template T
 */

But it creates a new type error in parse.

I think I got it working in TypeScript with a combination of your suggestion of the object index signature, plus an update on the return signature of the object overload to be conditional based on the object value.

I couldn't quite get it working in JSDoc, though I did make a bit of progress by making the @template tags type-constrained like in my earlier comment.

Maybe we're back at reconsidering TypeScript 😅

https://www.typescriptlang.org/play?#code/C4TwDgpgBAsghsAxgCwgJwGIDsA8GB8UAvFABRYD2AJhAFxQCiANhALYRbACUxhcWIANwAoUJFgIU6APIAjAFZ5CJAN5QA2gA96AZ2BoAllgDmAXXrwkqNHMUEoAHwlX02JVAC+w4QDMArliIwAYUWFBgcGg6EHhQEJrAHFQ6zlKYuPwg+AA0UNJxCUkplmm2Svjk1HSMLOycuayS1jr00lz0KuoA0lBGUADWECAUPnnmed2mBYlYyVAYUAD8UABKEMB+aFgAKuAx0pOErZMeIv6BwaHhkdGx8TNzJdZumfgVlDT0zGwcwA1N6Ba83aq3Wmx2eyUZwCQRCYQiURiC3uRVSzwyAhyeWmqKeMgU5UqnxqP3qUEaLiirUcwKgKmEUEZUDQYK2dNOwi8wkQoT0zIgOj8TGAxGuiNIVAoiD8dWAADpZNQQLk1D4KBQOlBZJF6ESIDwiIQPhA5YkEgBhUIzEUeTxcEQ8rB8lmC4UAJlFCOiEqlMt+CqVuVIiqoIANhBDIDlRiw6AAEtsYAAZe1AA

@johnhooks Looks like we both had the same idea this afternoon, I had also gone down this path of porting in 489bcdf, but it looks like you beat me by a few minutes. 😄

Check it out, the only thing that I couldn't figure out was how to run the tests.

The overloads have all the comments now, like they should.

It looks like you know how to register babel for tests with ts extensions! Awesome.

I'm going to pause on this. There are still some issues to resolve, but it seemed like I might have been stepping on your toes! I'll wait to hear from you. Also, it seemed like you were getting in to it, do you want to implement the same for rememo and memize, or would you like help?

@johnhooks No toe-stepping at all! I had dug a little deeper than I expected out of curiosity, and should have communicated earlier. I'm not too worried from my point-of-view about having duplicated the work, since it was an interesting TypeScript challenge. If you want to go ahead and work through those remaining issues, please feel free. Maybe there are some useful bits from my commit that you can use (e.g. I noticed that the object return type I mentioned in my earlier comment needed a bit more work to resolve the return type of deeply nested functions). My availability may be a bit spottier again over the next days, but if you reach a good stopping point, let me know and I'll try to make some time to help publish the finished version.

Similarly, I may not have the bandwidth to take a look at those other projects in the short term, so if you want to take a stab at it, I'd appreciate that.

This is now published as [email protected] 🚀