search

jpillora / node-edit-google-spreadsheet

A simple API for editing Google Spreadsheets
304 stars 101 forks source link

Node v4.0 compatibility : replace xml2json with xml2js #83

Closed gregaubert closed 8 years ago

gregaubert commented 9 years ago

Hi, I was unable to migrate my project to Node v4.0 because of the dependency to xml2json that is a lot of trouble due to its dependency to node-expat. So I replaced it with xml2js that is lighter and fully compatible with Node v4.0.

TimJohns commented 9 years ago

I'd like to see this change as well, but I just merged (I was at 0.2.16), and am having some trouble.

Heads-up that there may be a difference in the manner in which xml2json and xml2js handle numbers (at least as configured), which I believe is causing a problem here (line 441),:

      return e.status && e.status.code !== 200;

I'm investigating.

TimJohns commented 9 years ago

In order to be compatible with xml2json, xml2js will need to also be able to parse numerical attribute values to integers and floats instead of strings. I've added that functionality to that project in the form of an optional attrValueProcessor Parser parameter, and will be submitting a PR there shortly. Assuming that is accepted, I'll add a PR here and/or send @gregaubert an email with the related minor addition for this PR.

gregaubert commented 9 years ago

Ok thanks, let me know when you have news on this!

TimJohns commented 9 years ago

Here's the fork if you want to take a look at it or try it out:

https://github.com/TimJohns/node-edit-google-spreadsheet

The only line that is different from your fork is where I specify the new processor that I added to the fork of xml2js:

attrValueProcessors: [xml2js.processors.parseNumbers]

The package.json specifies that fork of xml2js (which I have submitted as the PR there).

I added some relevant unit tests, which are not yet run automatically by any CI system, but which you can run manually with 'npm test'.

Note that node-edit-google-spreadsheet fails in the same fashion with the latest xml2json (0.9.0), due to a change around 0.8.2 -- but can similarly be resolved by specifying coerce: true in the options.