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

Change documentation to recommend a different XML parser #344

Closed
xWyatt opened this issue Jul 14, 2022 · 10 comments
Closed

Change documentation to recommend a different XML parser #344

xWyatt opened this issue Jul 14, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@xWyatt
Copy link
Contributor

xWyatt commented Jul 14, 2022

Is your feature request related to a problem? Please describe.
The migration guide and README implicitly recommend using xml2js to parse XML output from itoolkit. xml2js doesn't appear to be actively maintained and has issues with parsing blank values from non-named keys which we have found to be an issue when getting returned values out of an RPG DS.

Describe the solution you'd like
I'd like to open a PR to change the docs to recommend a different XML parser. We have found fast-xml-parser to work quite well in lieu of xml2js and, in my opinion, the output JSON object is much cleaner (see below for screenshots).

Additional context
From xml2js, example output from an RPG DS with a 7s0 type with a 0 value. Note charkey _ exists in this object:
image

Also from xml2js, example output from an RPG DS with a 15a type and a "" value. Note charkey _ does not exist. This is inconsistent behavior:
image

From fast-xml-parser, the same 15a returns a blank value - this is what I expect:
image

@xWyatt xWyatt added the enhancement New feature or request label Jul 14, 2022
@kadler
Copy link
Member

kadler commented Jul 15, 2022

Sounds good to me.

@abmusse
Copy link
Member

abmusse commented Oct 13, 2022

I agree since its not really being maintained we should probably recommend a different parser.

Making a note here that we use xml2js in:

@alanseiden
Copy link
Contributor

Should we begin by making pull requests to update the code examples (e.g. pages like this one: https://nodejs-itoolkit.readthedocs.io/en/latest/ProgramCall.html#examples), replacing xml2js with the equivalent usage of fast-xml-parser?

@alanseiden
Copy link
Contributor

alanseiden commented Oct 20, 2022

The iToolkit package.json file includes xml2js as a dependency:

"xml2js": "^0.4.23"

Should we make a PR to add fast-xml-parser to package.json?

@abmusse
Copy link
Member

abmusse commented Oct 20, 2022

xml2js is still used in internal calls in Deprecated.js.
We cannot remove xml2js as a dependency until its replaced in all the internal calls.
We also use xml2js in tests.

@ThePrez @kadler Thoughts on gutting xml2js everywhere?

@abmusse
Copy link
Member

abmusse commented Oct 20, 2022

Should we begin by making pull requests to update the code examples (e.g. pages like this one: https://nodejs-itoolkit.readthedocs.io/en/latest/ProgramCall.html#examples), replacing xml2js with the equivalent usage of fast-xml-parser?

I believe @xWyatt Updated the code examples in this PR

@alanseiden
Copy link
Contributor

xml2js is still used in internal calls in Deprecated.js. We cannot remove xml2js as a dependency until its replaced in all the internal calls. We also use xml2js in tests.

@ThePrez @kadler Thoughts on gutting xml2js everywhere?

I was thinking to keep xml2js as a dependency while it's needed internally, but add fast-xml-parser to permit upgrading to the new component where needed.

@abmusse
Copy link
Member

abmusse commented Oct 21, 2022

xml2js is still used in internal calls in Deprecated.js. We cannot remove xml2js as a dependency until its replaced in all the internal calls. We also use xml2js in tests.
@ThePrez @kadler Thoughts on gutting xml2js everywhere?

I was thinking to keep xml2js as a dependency while it's needed internally, but add fast-xml-parser to permit upgrading to the new component where needed.

Yes adding fast-xml-parser as a dependency would be good.

@kadler
Copy link
Member

kadler commented Oct 24, 2022

@ThePrez @kadler Thoughts on gutting xml2js everywhere?

Sounds great to me. I think we wanted to do that initially but due to lack of time, we left it for the internal stuff.

@abmusse
Copy link
Member

abmusse commented Oct 26, 2022

internal

OK I will make an issue for this and tackle it in a follow-up PR

Edit:

See #350

This was referenced Oct 26, 2022
@abmusse abmusse closed this as completed Oct 26, 2022
abmusse added a commit that referenced this issue Oct 27, 2022
Add fast-xml-parser as an optional dependency now that we recommend it.

See #344 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants