Enhance type definition for XMLParser `isArray` option when using TypeScript

[plushie-cat]

• Are you running the latest version?
• Have you included sample input, output, error, and expected output?
• Have you checked if you are using correct configuration?
• Did you try online tool?
• Have you checked the docs for helpful APIs and examples?

Description

When creating an instance of XMLParser using the isArray option, the method arguments are currently defined as the following, with the jPathOrMatcher argument being either a string or Matcher type:

The behaviour as documented is that if the jPath option is set to true (or undefined since the default is true), then the type of jPathOrMatcher will be a string, and if jPath is false then the type of jPathOrMatcher will be a Matcher.

However, since the type of jPathOrMatcher is currently string | Matcher, to satisfy TypeScript I either have to add unnecessary conditional logic to check whether the type is string or Matcher, or add in a type assertion like (jPathOrMatcher as string).includes("blah").

I would like to propose updating the types so that it can smartly detect whether the jPath option is true or false, and set the type of isArray based on that. I've found that this can be achieved using union types, and I have a minimal working example using the TypeScript Playground:

https://www.typescriptlang.org/play/?#code/C4TwDgpgBAsghsAxgCwgJygXigbwFBSFRpwB2AJgPYC2ACmpZGqAFxRkgDceAvt3qEhQAGgCYAVgHkwwAJaVSAZyy4CRWYoCCaEiAD8bABTA4AcwBycahDaLgaWaVMAaKONoJkt+45dQNmsA+AEYArsA2UMGUlAA2EGQAlFgAfFEx8WTcRG4ewMgGUPahENw8UAA+qjkBOnD6RiYWVpF2Dk6u1Ago6GzwSKhorgFBDmERbNFxCaTJmGlTmaTZRO6ebABmcLGKpbz8APQHUAAqqEXg0JQbRedr+exopqHWpMD+pP5adSBf7FBtXx4RAKOy5TwnNAlaRyUFsMRSGTyJQqfA1b66RpmSzWVz3ZDDLSjWTjCBzNJonJENAQYChNCfYp7HI8ZxqQj4thM3h4PBHU7nQRXG75aD4x7PV7vRxfbS6P5wAE+JzA0HvfEAVQoEA2jgg5BhyMU8IkhtBqPZsp+WOauPB+UJgRC4TJqWqVMINLpDKKUOZRB4PL5xzO0CFUGutzFeWQEpeEDeHyt8o0-36PTQqqU6pjADFtrszUoTYjYSjsJTCLVMVBjNiWp1uoNHcTSeT3R6vfTGX6VoRWZbOVAtjsILwgA

Would you like to work on this issue?

• Yes
• No

Bookmark this repository for further updates. Visit SoloThought to know about recent features.

[github-actions]

We're glad you find this project helpful. We'll try to address this issue ASAP. You can vist https://solothought.com to know recent features. Don't forget to star this repo.

[amitguptagwl]

Would it not the case with other parameters too? Eg string | Uint8Array, alidationOptions | boolean etc?

[plushie-cat]

Oh I'm not sure but maybe that is the case - I'm only aware of this isArray case because I have code that is making use of it

I guess it might be a trade off between having a simpler typescript definition vs more complicated typescript definition but having the type be smart enough to match the implementation logic exactly 🤔

[hajin-song]

I'm wondering if it will be beneficial to create custom type to represent jPathOrMatcher type? If my understanding is correct, consumers of the repo will now need to add path-expression-matcher to their dependencies to satisfy the new typing.

For example,

export type PathOrMatcher = string | Matcher

...

  /**
   * Determine whether a tag should be parsed as an array
   * 
   * @param tagName - The name of the tag
   * @param jPathOrMatcher - The jPath string (if jPath: true) or Matcher instance (if jPath: false)
   * @param isLeafNode - Whether the tag is a leaf node
   * @param isAttribute - Whether this is an attribute
   * @returns {boolean}
   * 
   * Defaults to `() => false`
   */
  isArray?: (tagName: string, jPathOrMatcher: PathOrMatcher, isLeafNode: boolean, isAttribute: boolean) => boolean;

[amitguptagwl]

I believe defining Matcher and Expression as unknown would be better.

[amitguptagwl]

I've published changes in v5.5.4. Now you'll not need to install path-expression-matcher until you want to use it

[amitguptagwl]

You can add you test case in following repository for understanding purpose if it is still not supported

https://github.com/NaturalIntelligence/fxp-type-testing

[plushie-cat]

@amitguptagwl sorry maybe I'm not quite understanding but would you be able to elaborate on why unknown is better? With unknown it relies on the consumer to know the behaviour of the implementation and an error caused by accidentally using the wrong type would not get caught until runtime.

Apologies if I'm not understanding your point but I'd just like to know what is the benefit of using unknown as the type instead of the solution proposed by @hajin-song to have a custom PathOrMatcher type, or making the type of isArray aware of what the jPath boolean option is and setting the type based on that using the union type approach that I mentioned in my original comment

[amitguptagwl]

unknown for new library. So user is not forced to install new library just to ignore compilation error while they're not using that feature. Though not recommended.

I've not done any change to previous typings. Because it'll make typings more complex and most of the patches of this library were to fix typings only :)

[KuSh]

For the record, unknown still cause compilation errors :

const options: X2jOptions = {
  isArray: (_tagName, jPath, _isLeafNode, _isAttribute) => ["mypath1", "mypath2"].includes(jPath), // error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.
}

[amitguptagwl]

Well the type of jPath in your case should be string | unknown. Not only unknown.

[KuSh]

That's the case, but unknown can't be used as parameter of Array.includes() as it only accept string

[amitguptagwl]

I understand. The issue is I don't want to force the user to install path-expression-matcher just for the typings. BUT if I do that then this is the issue.

Can you please type cast it until I find some better solution?