[main] improve passing of input data to output

[pkra]

Resolves #303

[pkra]

Not sure if this is too simplistic.

[dpvc]

I don't think this is the right approach, as it means that you are modifying the original data object (to add in the defaults), which is not a good idea, and since that object is part of the calling code, it could be modified outside of mathJax-node before mathjax-node has processed it. That is one reason the mathjax-node currently copies the data rather than keep it directly.

Instead, I'd recommend adding a copy of the data object to the information stored in the queue (at line 837), as

  queue.push([options,callback,Object.assign({},data)]);

and then at line 662 save the original as originalData:

 data = item[0]; callback = item[1]; originalData = item[2];

and return that at line 776

  callback(result, originalData);

You will want to clear originalData at line 650, and define it originally at line 88.

[pkra]

I don't think this is the right approach, as it means that you are modifying the original data object (to add in the defaults), which is not a good idea, and since that object is part of the calling code, it could be modified outside of mathJax-node before mathjax-node has processed it.

Right. But Object.assign({},data) give us a shallow copy as well, no? So we'd still have that risk, just somewhat reduced.

(Also, a separate PR may have been shorter 😉 )

[dpvc]

But Object.assign({},data) give us a shallow copy as well, no?

Yes, but the only nested structure that is part of the data object is the state object, and the is supposed to update as we go, so I'm not sure a copy of that is necessary. In any case, the copy is not being used by mathjax-node; it is only kept in order to pass it back in the callback, so the risk to mathjax-node is entirely gone (it was the options = data that was the source of the risk).

If you want a deep copy, then use Insert({},data) instead. I'm fine either way.

(Also, a separate PR may have been shorter 😉 )

but would not have constituted a code review, and would not have said why the changes were suggested, and would have required me to spend time testing it to make sure it works. So it would have been shorter for you, but longer for me. 🙂

[pkra]

@dpvc updated as per your suggestion. PTAL.

[dpvc]

Can you remove the leading space that was added?

[dpvc]

Also here?

[dpvc]

Other than the unwanted spaces, I think it is OK.

[dpvc]

LGTM

[zorkow]

General remark on tests:
I think eventually they should be combined into a dedicated test runner, to avoid duplication and maintenance costs.

[zorkow]

Don't double park instructions.

[zorkow]

Object.assign is ES6. Is that intended?

[pkra]

@zorkow Where's this coming from? This PR has already been reviewed by @dpvc so I wasn't planning any more change (just delaying for another bug fix release before a feature release with this).

I think eventually they should be combined into a dedicated test runner, to avoid duplication and maintenance costs.

Please file an issue. (Though, as you know, I don't see the need at this point.)

Don't double park instructions.

Let's please do a style clean-up some other time. A separate issue might be good.

Object.assign is ES6. Is that intended?

Since Node v4 supports it (cf. the passing test), I don't see an issue.

[pkra]

Ok, sorry for the mess of half-borked branches while working out that my local develop branch had been screwed up.

This is now a clean new branch off develop with the same changes as an "earlier" commit.

I've now fixed two things I missed from @dpvc's earlier suggestion -- declaring and clearing originalData.

[pkra]

@dpvc if you can take another look just for sanity's sake, I'd appreciate it. These are the same changes (plus two things I overlooked from your earlier recommendations).

[dpvc]

LGTM