search

NaturalIntelligence / fast-xml-parser

Validate XML, Parse XML and Build XML rapidly without C/C++ based libraries and no callback.
https://naturalintelligence.github.io/fast-xml-parser/
MIT License
2.49k stars 302 forks source link

Introduction of eNotation parameter breaks existing code #538

Closed marcello-morena closed 1 year ago

marcello-morena commented 1 year ago

Description

Hi. Just want to report that with yesterday's commit (45b01db) the new mandatory field eNotation was introduced in the numberParseOptions configuration parameter. This breaks ALL existing code for projects using Typescript and this particular configuration parameter, since it makes tsc fail.

Moreover, there was no mention in the docs of what eNotation does and the change is introduced in a patch version update (while breaking changes should only appear in major version updates).

Unfortunately for us, we discovered this change in the worst possible way: a client, officially testing the code that we delivered two days ago, calling us this morning saying that the code wasn't even compiling and demanding an emergency fix (that we promptly provided).

So, we fixed this on our side, but I strongly suggest that you release a new version making eNotation optional.

On a final note: please don't get me wrong, we really DO appreciate all the hard work that you put into this project, but this was a major headache for us.

github-actions[bot] commented 1 year ago

I'm glad you find this repository helpful. I'll try to address your issue ASAP. You can watch the repo for new changes or star it.

amitguptagwl commented 1 year ago

Thanks for your feedback @marcello-morena . We run all tests before any release. But as the library is used in different ways by different projects, it might have broken your experience. I would appreciate if you raise a PR and add your usecase in the tests so the same situation can be avoided in future.

I would be releasing another minor version today to make it optional.

amitguptagwl commented 1 year ago

done

tbouffard commented 1 year ago

The type is now optional in v4.0.15 thanks to this commit: 6ebcb14509d351c27ec525f0380e736e0c058f75 However, this commit now set the default value of eNotation to false, which breaks the previous behavior. Previously, when the eNotation wasn't set, fxp used the default value of strnum which is true. See https://github.com/NaturalIntelligence/strnum/blob/3fcb88602a08c103bccffc8c36041c3f3d0f8317/strnum.js#L16-L22

I can provide a PR to reproduce the problem. Please let me know. I have detected the behavior change here: https://github.com/process-analytics/bpmn-visualization-js/pull/2494#pullrequestreview-1275179961

amitguptagwl commented 1 year ago

Yes, you're right. I'm launching 4.1.0 soon to fix it.

tbouffard commented 1 year ago

Thanks @amitguptagwl for the quick feedback and the planned release. For people using v4.0.15 and that want to keep the previous behavior, just set the eNotation property to true in the numberParseOptions.