chore: switch to new xmldom and fast-xml-parser #3920
Conversation
vasicvuk
requested review from
joshgummersall,
stevengum and
a team
as code owners
|
I also have removed the xml2js as fast-xml-parser works both with Node and Browser. I appended the XML header and trimmed the newline so that tests work without modification as they did for NodeJS. |
|
For posterity, @vasicvuk can you confirm that this package works well in the browser? Running |
joshgummersall
changed the title
|
Looks like |
Pull Request Test Coverage Report for Build 1218600997
💛 - Coveralls |
|
Sure, i will remove the typings, and check in the browser ones i get to PC later |
|
Hi @joshgummersall, I removed typings and also tested if the changed worked in browser. I used following HTML for testing: <html>
<head>
<script src="lib/browser.js"></script>
</head>
<body>
<script>
const scope = {};
var expression = AEL.Expression.parse('xml(\'{"person": {"name": "Sophia Owen", "city": "Seattle"}}\')'.toString());
const value = expression.tryEvaluate(scope);
console.log(value);
</script>
</body>
</html>As a result i got this: which confirms that it works in browser too. |
|
@vasicvuk, please remove |
|
@joshgummersall I can remove the package lock but there is no yarn.lock in libraries/adaptive-expressions. Root folder yarn.lock will remain same as nothing changes when I run the command. |
|
@vasicvuk, yep that's expected! One |
|
@joshgummersall |
|
It looks like |
|
Okey. I just noticed that build passes when I return it and I get same error when I don't have it. Here are logs: You can see a build passing after yarn command. This is when I added types. |
|
Huh - well, I suppose you can go ahead and add the types back. You'll have to add |
|
I have added it again and included it in .depcheckrc file since --ignores flag seems to be ignored if .depcheckrc file provided. Also I have run depcheck to verify it and run build on clean repository and everything passes. |
|
Fixed it now, had to add also sinon as dependency as it is used within tests. I had this error: |
|
Please remove the added |
|
@joshgummersall done |
|
@cosmicshuai can you take a look at this as well? Just want a second set of eyes and you've worked on |
|
@joshgummersall New version of x2js is also released. So I can also now provide a PR with less changes, but I think it is a better option to change it to fast-xml-parser as it has more Contributors, Stars on GitHub, Downloads, it is used in other Microsoft OSS and other large companies too. |
|
Agreed, I was hoping to get another set of eyes on the change but I’m inclined to move forward either way. I’ll land this today either way. |
|
fast-xml-parser have issue now too |
Fixes #3916
Description
Since x2js did not respond to PR I have changed the library to fast-xml-parser which is far more maintained and then I have updated xmldom to latest @xmldom/xmldom
Specific Changes
Testing
All existing tests are passing as no functionality is changed