Fix xml parser prior to upgrading to node v22 #2656
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-4729 We want to upgrade to node v22, but we have a blocker with the xml parser we are using. The tool being used is libxmljs and has some open issues: libxmljs/libxmljs#664 libxmljs/libxmljs#660 Mainly the issues are: - is it being maintained - is there going to be a release to make it work with node v22. Rebuilding the package locally during our npm install has not worked, (through minimum effort) and if we did make it to build there is no guarantee it will actually work when built. This spike investigates the effort required to get any xml parser to work with the least possible effort. The change suggest using libxmljs2. Which works. However, if you look at the documentation it states : "LibXML bindings for node.js This package was forked as the original one is fairly unmaintained." Sounds good. But no it clearly now states it is "NO LONGER MAINTAINED". This can be a starting point to make a decision and we should not commit this branch. https://www.npmjs.com/package/xml-js looks a lot more popular and could do with a spike to assess the effort required to replace the existing parser. Lets discuss.
jonathangoulding
added
housekeeping
jonathangoulding
added a commit
that referenced
this pull request
Nov 21, 2024
https://eaflood.atlassian.net/browse/WATER-4729 We were previously blocked from upgrading to node to v22 by a xml parser package not supporting it. The previous work to resolve this has been done here - #2656
Merged
jonathangoulding
changed the title
Cruikshanks
requested changes
Nov 21, 2024
I will mark as draft. For proof of it working i have created a v22 branch - #2657 |
Cruikshanks
approved these changes
Nov 21, 2024
from me!
|
On your wider point @jonathangoulding , this is one for the business. Essentially, our current implementation is not only unmaintained but also has vulnerabilities that will never be resolved. If we are to continue offering returns submission via XML, then we must However, knowing how little it is used, they may opt to work with the one user to help them migrate to one of our alternate methods. We can then drop the function and dependency altogether. I'll write something up so we have a chance of this issue not being forgotten. |
jonathangoulding
added a commit
that referenced
this pull request
Nov 21, 2024
https://eaflood.atlassian.net/browse/WATER-4729 We were previously blocked from upgrading to node to v22 by a xml parser package not supporting it. The previous work to resolve this has been done here - #2656
jonathangoulding
added a commit
that referenced
this pull request
Nov 22, 2024
https://eaflood.atlassian.net/browse/WATER-4729 We were previously blocked from upgrading to node to v22 by a xml parser package not supporting it. The previous work to resolve this has been done here - #2656
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4729
We want to upgrade to node v22, but we have a blocker with the xml parser we are using.
The tool being used is libxmljs and has some open issues:
Mainly the issues are:
Rebuilding the package locally during our npm install has not worked, (through minimum effort) and if we did make it to build there is no guarantee it will actually work when built.
This spike investigates the effort required to get any xml parser to work with the least possible effort.
The change suggest using libxmljs2. Which works.
However, if you look at the documentation it states : "LibXML bindings for node.js This package was forked as the original one is fairly unmaintained."
Sounds good. But now it clearly now states it is "NO LONGER MAINTAINED".