breaking typing changes in v4.3.0

[jeremymeng]

after upgrading we encounter the following error from TSC

src/xml.ts(102,7): error TS7053: Element implicitly has an 'any' type because expression of type '"?xml"' can't be used to index type 'GenericObjectOrArray<unknown>'.
  Property '?xml' does not exist on type 'GenericObjectOrArray<unknown>'.
src/xml.ts(103,12): error TS7053: Element implicitly has an 'any' type because expression of type '"?xml"' can't be used to index type 'GenericObjectOrArray<unknown>'.
  Property '?xml' does not exist on type 'GenericObjectOrArray<unknown>'.
src/xml.ts(108,21): error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'GenericObjectOrArray<unknown>'.
  No index signature with a parameter of type 'string' was found on type 'GenericObjectOrArray<unknown>'.

in previous version, parse() returns any so we were able to index it

  const parsedXml = parser.parse(str);

  // Remove the <?xml version="..." ?> node.
  // This is a change in behavior on fxp v4. Issue #424
  if (parsedXml["?xml"]) {
    delete parsedXml["?xml"];
  }

[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.

[qiulang]

I also hit this problem and I have to resort to const parsedXml = parser.parse(str) as any but that just defeats the purpose of introducing unknown in 4.3.

But the problem is I am not sure where the xml string contains a specfic tag or not, so the code if (parsedXml["?xml"]) {...} is just to check whether that tag exists. Now with unknown how to I do that now?

[jeremymeng]

@qiulang maybe you can try parser.parse<Record<string, any>>(str)?

[amitguptagwl]

I merged the PR without thinking other negative possible cases. I believe I should have some tests to check the typings. Anyway, I'm planning to revert this change as warning is better than error.
Let me know your opinion/

[qiulang]

@amitguptagwl if we decide to use unknown and do not cast into any as I did, the only solution probably will be using in like this

if (parsedXml  && typeof parsedXml === "object" && 'some-tag' in parsedXml) {
    //access some-tag at parsedXml['some-tag']
}

I really don't think that adds much value (or "safety") to just if (parsedXml["?xml"]) in 4.2.2 (and just to delete it in the end). I always think ts doesn't do a good job for in operator as discussed in microsoft/TypeScript#21732 and microsoft/TypeScript#43284

And the SO discussion How do you validate properties on an unknown typed parameter in typescript? and How to check for the property type of an unknown value?

But that is just my opinion.

[amitguptagwl]

let me first revert the changes. and discuss for the right solution before applying change

[jdforsythe]

This project constantly has breaking type changes in patch and minor versions. Are there any plans to review type changes before releasing?

[qiulang]

Reporting the issue and providing suggestions is probably what we can do best.

[amitguptagwl]

@jdforsythe This was the 2nd incident happened this year related to typings. If you want to highlight anything particular, it would be helpful.
Currently, there is only 1 maintainer. But It would be really helpful if you can help in reviewing the PRs.
In general, I rely on UTs, and typings are not the part of that. So any suggestion or PR to add them in testing would be appreciable.

[jdforsythe]

@amitguptagwl I went back through our PRs and I see 6 breaking changes since 4.0.0 - the changes we had to make are listed below:

• (between 4.0.0 and 4.0.7 sometime, we skipped a few versions) - added required ignoreDeclaration: true and ignorePiTags: true
• from 4.0.8-4.0.9 - added required transformTagName: false
• from 4.0.12 to 4.1.1 - added required transformAttributeName: false
• from 4.1.3 to 4.2.2 - added required updateTag: (tagName: string, _jPath, _attrs) => tagName function
• from 4.2.7 to 4.3.0 - type changed and needed to pass a type to parser.parse<ResultingType>() instead of previous type casting <ResultingType> parser.parse()
• from 4.3.0 to 4.3.1 - the type change from the previous migration had to be reverted back to <ResultingType> parser.parse()

So to be fair, there have been only 2 truly typing-based breaking changes - the rest have been actual breaking changes in the code for non-major version bumps.

I'd be happy to work on it with you.

[amitguptagwl]

Thanks for your kindness.

Did you mean to say that a new feature should not be added as minor version even if it is not a breaking change?

[jdforsythe]

Did you mean to say that a new feature should not be added as minor version even if it is not a breaking change?

I want to be as accurate as possible - I'll show the example of adding transformTagName option in 4.0.9.

Here's the commit adding that option:
6ad40ee

From a Javascript perspective, this looks like it should have been a minor version bump - since it's adding a new feature, but the default behavior stays the same and doesn't break existing code.

From a TypeScript perspective, this was a breaking change.

6ad40ee#diff-7d7baa1d792f434767bad0726a44843fb4f396efcf889968fb18cc5578f03809

This adds a new required transformTagName property to X2jOptions type. Since this property didn't exist prior to 4.0.9, nobody using types has it in their X2jOptions objects, but now the type requires it to be present, so anybody using this library with TypeScript now has failing builds due to a missing property in their options.

To fix this, we had to add transformTagName: false to our options object. This didn't change the behavior of the library, since that is the default, but it still causes the TypeScript build to fail because the new property is required.

Instead, the property should have been added as optional, then it would have been a truly non-breaking change - our code would have continued compiling and behaving as it was before without modifying our code at all.

Adding this ? to make the new property optional would have made the type line up with the actual JavaScript behavior:

type X2jOptions = {
  // ...
  ignorePiTags: boolean;
  transformTagName?: ((tagName: string) => string) | false;
};

--

This is a similar issue that happened with the addition of other features/options, such as transformAttributeName, updateTag, etc.

Are the types updated by hand? I should be able to come up with a test that will just compile some TypeScript and error on breaking changes. I can come up with something and submit a PR.

[amitguptagwl]

I got you and it is really helpful. Since most of the options are kind of optional so we can update typings. Yes, types are updated manually.

[jdforsythe]

@amitguptagwl I submitted a PR with tests to ensure the types are set as optional going forward. This should prevent these issues in the future.