onLoadFinished arguments wrong with SlimerJS

[awlayton]

SlimerJS passes 3 arguments to the onLoadFinished callback, while PhantomJS only passes 1.

If one registers an onLoadFinished callback taking 1 argument and is using SlimerJS, the callback will be called with an array of the 3 arguments as the first argument.

I believe the problem stems from this section. It appears this issue will happen whenever the callback has one argument but is called with multiple arguments.

This is related to johntitus/node-horseman#180

[puzrin]

Suggestions? No idea why that magic with wrap/unwrap arrays was done, looks suspicious. It existed in v1.0 and we did not changed anything there. I have no principal objection to bump version to 3.0 and remove all legacy mess.

@baudehlo may be you remember something about that code :) ?

[awlayton]

My suggestion would be to always use .apply rather than .call, like you said. I also think it looks like it is doing something weird...

[baudehlo]

I have no recollection of anything - sorry - I haven't worked on this code in years.

On Thu, May 12, 2016 at 10:43 AM, Alex Layton notifications@github.com wrote:

My suggestion would be to always use .apply rather than .call, like you said. I also think it looks like it is doing something weird...

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/baudehlo/node-phantom-simple/issues/133#issuecomment-218779300

[puzrin]

@awlayton could you do a PR with test?

[awlayton]

I am working on a paper right now. I might be able to get to it this weekend, maybe.

[awlayton]

I tried to make a test, but now I can't get the bug to happen @puzrin. What SlimerJS were you using @winghouchan?

[winghouchan]

I'm using SlimerJS 0.10.0 @awlayton.

[winghouchan]

I have a feeling that the problem caused by using .call rather than .apply in this situation is a lot more expansive. This is coming from a case I just witnessed with a callback on the consoleMessage event. When running the callback in PhantomJS, 3 arguments are expected:

const webPage = require('webpage');
const page = webPage.create();

page.onConsoleMessage = (msg, lineNum, sourceId) => {
  console.log(`CONSOLE: ${msg} (from line #${lineNum} in ${sourceId})`);
};

However a consoleMessage callback in SlimerJS gets passed the first argument with an array of elements corresponding to each expected argument.

Perhaps more investigation is required to see how far this issue expands exactly.

[awlayton]

Yes the problem potentially affects all callbacks @winghouchan. Perhaps you have more time to make a test and fix for this issue?

[winghouchan]

I'm certainly open to putting a PR together for this. However my time is also limited so it won't be something I'm prioritising right now.

[puzrin]

After last surprise with banned slimerjs downloads on travis, i decided to spend some resources to significantly improve things. And we transparently migrated our testing lib to electron :) . https://github.com/nodeca/navit (now in dev branch). My impression is that Electron is much more stable and predicatable. Slower than phantom, but faster than slimer.

[y0hnn]

Can we except a merge ?