Replace Expat dependency with Sax

[christiaanwesterbeek]

@jsdevel I followed your steps. Here's the pull request for my feature branch that's based on the current master of milewise/node-soap. Here I replaced the expat dependency with sax. Refs issue #206

[jsdevel]

Can you rebase your feature branch onto master so there's only one commit?

[christiaanwesterbeek]

Is this the procedure to do that? http://stackoverflow.com/a/9625775/1385429
(I'm learning here, sorry to trouble you)

[jsdevel]

No worries! I'm learning too.

Essentially yes. That's all there is too it; however you may want to use git rebase -i master on your feature branch instead. Then you'll git push -f origin featurebranch in your feature branch to update your PR. There's no need to checkout master and merge your feature branch. Just make sure master is up to date before rebasing.

Doing things this way allows you to work on multiple features at once.

[christiaanwesterbeek]

Sounds good. I'm giving it a go now.

[christiaanwesterbeek]

I'm going to need a coach here I'm afraid and I need to be on another job right now. Will try again tomorrow.

C:\Users\Christiaan\Documents\GitHub\node-soap>git checkout feature-sax
Branch feature-sax set up to track remote branch feature-sax from origin.
Switched to a new branch 'feature-sax'

C:\Users\Christiaan\Documents\GitHub\node-soap>git remote show origin
* remote origin
  Fetch URL: https://github.com/devotis/node-soap.git
  Push  URL: https://github.com/devotis/node-soap.git
  HEAD branch: master
  Remote branches:
    feature-not-override-attributes tracked
    feature-sax                     tracked
    master                          tracked
  Local branches configured for 'git pull':
    feature-sax merges with remote feature-sax
    master      merges with remote master
  Local refs configured for 'git push':
    feature-sax pushes to feature-sax (up to date)
    master      pushes to master      (up to date)

C:\Users\Christiaan\Documents\GitHub\node-soap>git rebase -i master
error: could not apply cd524a7... remove expat as a dependency and add sax

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply cd524a7... remove expat as a dependency and add sax

C:\Users\Christiaan\Documents\GitHub\node-soap>git rebase --continue
benchmark.js: needs merge
You must edit all merge conflicts and then
mark them as resolved using git add

C:\Users\Christiaan\Documents\GitHub\node-soap>

[christiaanwesterbeek]

I'm being unsuccessful. Need to read the tutorials word for word. Git's pretty new to me. Unfortunately I don't have a coach here present, so this will take some more time.

Is the screenshot below showing the end result I'm working towards?

[jsdevel]

@devotis So you had 6 commits in master then 3 made in feature-sax. Here's what I did to correct the issue:

git clone https://github.com/devotis/node-soap.git devotis-soap
cd devotis-soap
git reset --hard HEAD~6
git checkout feature-sax
git rebase -i HEAD~9

During the rebase, it's safe to mark every commit after the first one as s (or squash). Then when you're prompted to edit the history, choose the commit message that states the purpose of the feature branch. In your case it's "remove expat as a dependency and add sax".

At this point you'll have to git push -f origin feature-branch. That should update your pull request as well.

[christiaanwesterbeek]

@jsdevel YES! You are awesome. I'll make sure to continue reading the tutorials on Git. I guess it's ready to be pulled now.

[jsdevel]

@devotis Thanks! I'm still learning too, so I appreciate the opportunity to help. I'm also glad @aheckmann has been requiring rebases to keep history clean.

One thing about your commit that I see as an item leftover from the benchmark is your use of arguments. Can you change each instance of:

p.onopentag = function(node) {
  var nsName = arguments.length === 2 ? arguments[0] : node.name;
  var attrs  = arguments.length === 2 ? arguments[1] : node.attributes;

To this:

p.onopentag = function(node) {
  var nsName = node.name;
  var attrs  = node.attributes;

[christiaanwesterbeek]

True. Done. and I rebased again.

[jsdevel]

Awesome. I'll pull this into https://github.com/godaddy/node-soap tonight probably. Thanks!

[jsdevel]

@devotis I pulled your changes into https://github.com/godaddy/node-soap. Worked great with our other projects so far! Thanks.

[christiaanwesterbeek]

You too. This was inspiring.

[wprater]

why was this replaced? we are having performance issues with SAX and Im curious why node-expat was removed?

[herom]

As you may have recognized, this PR is more than 2 years old - I don't know exactly why this was changed, but there are chances that the performance issues you're seeing were maybe not present then...

Which performance issues have you tracked while using the sax package?

[wprater]

I assume it was probably less about performance and more about not using a native dependency?

On our larger SOAP responses, performance of saxjs is more than 5 times slower when compared to easysax or node-expat. We've tried both and now are using a fork with easysax!

xmlToObject
saxjs: 98.2ms
node-expat: 29.9ms
easysax: 17.1ms

[herom]

This is really a huge performance boost 👍 Are you interessted in contributing and send in a PR with the replaced easysax dependency? This would be awesome 💃