These are chat archives for rosshinkley/nightmare

12th
May 2016
Rick Medina
@rickmed
May 12 2016 00:21
hey guys, I see in nightmare's home page: "which is similar to PhantomJS but faster and more modern." have you benchmarked ?
Ross Hinkley
@rosshinkley
May 12 2016 00:24
@reinpk did a while back
don't know if those numbers still stand
Rick Medina
@rickmed
May 12 2016 00:32
@rosshinkley cool, thanks
Ross Hinkley
@rosshinkley
May 12 2016 00:32
no problem :)
Rick Medina
@rickmed
May 12 2016 00:32
had now idea nightmare was originally based in phantom
btw, have you decided the way to go about the iframes stuff?
Ross Hinkley
@rosshinkley
May 12 2016 00:34
no, not really
it's not a trivial problem to solve with nightmare as it is
Rick Medina
@rickmed
May 12 2016 09:17
guys what's up with these warnings? (node) warning: possible EventEmitter memory leak detected. 11 error listeners added. Use emitter.setMaxListeners() to increase limit.
Also, question: does each instance of nightmare runs on a child process? what happens if an error occurs in any of them?
Ross Hinkley
@rosshinkley
May 12 2016 14:01
@rickmed i'll put that toward the top of my list for next things to look at, i know that's also been a nagging problem

does each instance of nightmare runs on a child process?

Yes.

what happens if an error occurs in any of them?

An error in one should not cause problems in another. Instances are isolated. (at least for now.)

Rick Medina
@rickmed
May 12 2016 16:03
@rosshinkley but is it a legit memory leak?
Ross Hinkley
@rosshinkley
May 12 2016 16:41
i remember there being a legitimate problem that was pointed out and never pr'd
but there are probably other event emitter leaks, yeah
Rick Medina
@rickmed
May 12 2016 17:07
with every queued action is there a listener registered for a response event in the electron process? is that how it works?
Ross Hinkley
@rosshinkley
May 12 2016 17:10
memory serving
@Mr0grog wrote some beautiful sugar around that to prevent event crosstalk
but at the core, yes, you're adding a listener and using EventEmitter.once() to listen for a response
i don't think that's what's causing the problem, though
Rick Medina
@rickmed
May 12 2016 17:12
what do you think is?
Ross Hinkley
@rosshinkley
May 12 2016 17:12
as those are executed in sequence
not sure, haven't had a chance to look into it yet
Rick Medina
@rickmed
May 12 2016 17:13
but let's say you have +10 queued actions, for each (at least?) one listener, wouldn't easily surpass the default for ten in nodejs triggering the warning?
Ross Hinkley
@rosshinkley
May 12 2016 17:13
the listeners aren't made until the queued action is executed, though
Rick Medina
@rickmed
May 12 2016 17:14
yeah, that was my next question
mmmm....
Ross Hinkley
@rosshinkley
May 12 2016 17:14
i've got a couple of minutes, i'm trying to find that original issue
Rick Medina
@rickmed
May 12 2016 17:15
i'm reading this segmentio/nightmare#350 and segmentio/nightmare#282
there was a merged PR but only cleaning up after ending
Ross Hinkley
@rosshinkley
May 12 2016 17:15
350 is the one i was thinking of
specifically this comment
errr
sorry, this comment
and yeah, the process listener cleanup is important
and there are probably other small problems like the one described in 350
282 is an interesting case
Rick Medina
@rickmed
May 12 2016 17:19
there wasn't a PR of sorts like this fix right? https://github.com/segmentio/nightmare/issues/350#issuecomment-158407701
Ross Hinkley
@rosshinkley
May 12 2016 17:19
not that i know of
Rick Medina
@rickmed
May 12 2016 17:20
yeah, I think 282 with concurrent nigthmare instances is a complete different monster...
I think I read an issue somewhere about ideas how to manage nightmare instances/processes...
Ross Hinkley
@rosshinkley
May 12 2016 17:21
oh boy.
there are several of those
segmentio/nightmare#593 mentions it
but i can't remember off the top of my head where else it's been talked about
Rick Medina
@rickmed
May 12 2016 17:23
but what I'm referring is just single instance, with very simple (like 15 actions) scripts I get the memory leak warning, I think it is a legit one
Ross Hinkley
@rosshinkley
May 12 2016 17:23
yeah, that's not great and should be fixed
i have to go for a bit, i'll be back
Rick Medina
@rickmed
May 12 2016 17:24
I'm gonna do quick patch/test with this https://github.com/segmentio/nightmare/issues/350#issuecomment-158407701 to see if it works
Rick Medina
@rickmed
May 12 2016 17:41
so i made i change without formal testing (did a before/after test on my original script and it worked)
so on line 246 of runner.js
parent.respondTo('javascript', function(src, done) {
    renderer.once('response', function(event, response) {
      done(null, response);
    });

    renderer.once('error', function(event, error) {
      done(error);
    });

    renderer.once('log', function(event, args) {
      parent.emit.apply(parent, ['log'].concat(args));
    });

    win.webContents.executeJavaScript(src);
  });
for the javascript event (which I think is used for most/all? of the actions) when there is no error the error listener is not removed with once
and viceversa with response
so
  parent.respondTo('javascript', function (src, done) {

    function response (event, response) {
      renderer.removeListener('error', error);
      renderer.removeListener('log', log);
      done(null, response);
    }

    function error (event, error) {
      renderer.removeListener('response', response);
      renderer.removeListener('log', log);
      done(error);
    }

    function log (event, args) {
      parent.emit.apply(parent, ['log'].concat(args));
    }

    renderer.once('response', response)

    renderer.once('error', error);

    renderer.once('log', log);

    win.webContents.executeJavaScript(src);
  });
I changed to that and it worked, what do you think?
Rick Medina
@rickmed
May 12 2016 17:56
I tried without removing the log event but the warning appeared again, so maybe it is not being called on response or error?
Ross Hinkley
@rosshinkley
May 12 2016 18:05
uh, at first blush, removing the log event is reasonable
also, shouldn't that be renderer.on('log', log)?
(you could have >1 log call)
Rick Medina
@rickmed
May 12 2016 18:11
where? that's what I have, no?
oh wait
well, that is how it is originally
but not sure if it there are somewhere +1 log calls
Ross Hinkley
@rosshinkley
May 12 2016 18:13
oh , sorry
yeah, i realize that's hwo it is now
but it just jumped out at me just now
you could conceivably have like...
hm
now i'm doubting myself
i know the console.log calls are hooked up via the ipc
but i can't remember how or where the log calls for javascriptare done offhand, i'd have to go look
that's a good question :)
Rick Medina
@rickmed
May 12 2016 18:17
well, I guess that's a starting point ...:)
another question
Ross Hinkley
@rosshinkley
May 12 2016 18:18
shoot
Rick Medina
@rickmed
May 12 2016 18:20
for example, let's say I have a selector in an action that will no exist in the page, shouldn't that err somehow? I made a test with type and it just closed the nightmare instance after sometime (a default timeout I suppose)
Ross Hinkley
@rosshinkley
May 12 2016 18:20
it does... how that error manifests depends on how you're running nightmare
well
i should say, it should error out :P
eg, if you're using .then(), the error won't be an exception, it'll be passed to a .catch() handler
Rick Medina
@rickmed
May 12 2016 18:22
ohhhhh, wait a sec
Ross Hinkley
@rosshinkley
May 12 2016 18:22
using .run(), it'll be the error parameter in the callback
etc
the best situation i can think of where it'll be a for-serious thrown exception is using vo/co
i think
this is all from the hip
Rick Medina
@rickmed
May 12 2016 18:24
but using .then instead of run is the preferred way now, right?
Ross Hinkley
@rosshinkley
May 12 2016 18:25
"preferred"
yes
Rick Medina
@rickmed
May 12 2016 18:26
I was missing a catch, then I catched the error (timeout after 30000ms) and the call stack but nothing more (tested a not existing element in type), is that the current functionality?
Ross Hinkley
@rosshinkley
May 12 2016 18:26
for completeness, have a look at segmentio/nightmare#575
what were you doing that caused the error?
Rick Medina
@rickmed
May 12 2016 18:27
tested a not existing element selector in type
Ross Hinkley
@rosshinkley
May 12 2016 18:27
hm
i'd think .type() would error immediately
Rick Medina
@rickmed
May 12 2016 18:28
i see .then is using internally run? does it forward the same error that run would throw?
Ross Hinkley
@rosshinkley
May 12 2016 18:28
yes
out of curiosity...
what version of nightmare are you running?
Rick Medina
@rickmed
May 12 2016 18:29
IDEM with click()
latest
wait
yes
2.4.0
can you please remind me or point me to an example how to use run vs then?
Ross Hinkley
@rosshinkley
May 12 2016 18:31
sure
first
the unit tests seem to agree with me that type should error
and 30s is double or triple the mocha.opts timeout
regarding your question about examples, hang on
same example written two different ways
hopefully that's helpful to you :)
Rick Medina
@rickmed
May 12 2016 18:34
let me do a quick test
Ross Hinkley
@rosshinkley
May 12 2016 18:34
back to the problem at hand, if you have a minimum example, i'd like to see it
Rick Medina
@rickmed
May 12 2016 18:35
about the not existing selector?
Ross Hinkley
@rosshinkley
May 12 2016 18:35
yep
Rick Medina
@rickmed
May 12 2016 18:37
the error should be in the lines of "Cannot read property 'blur' of null"?
Ross Hinkley
@rosshinkley
May 12 2016 18:38
i think so
i know that's not super-helpful right now
up to 2.4.0 (or maybe the version just prior), it didn't error at all
i fixed it temporarily, but @yoz added some better blur/change management stuff that was way better
Rick Medina
@rickmed
May 12 2016 18:39
not super helpful! :smile: :stuck_out_tongue_winking_eye:
maybe an upgrade for next version
Ross Hinkley
@rosshinkley
May 12 2016 18:39
file it! ;)
Rick Medina
@rickmed
May 12 2016 18:39
ok
luckily we have visual feedback so one would not be completely lost
I didn't have the error before bc I was doing somethin else sorry :)
Ross Hinkley
@rosshinkley
May 12 2016 18:42
no problem
Rick Medina
@rickmed
May 12 2016 18:42
also, why there is a default of alwaysontop window?
Ross Hinkley
@rosshinkley
May 12 2016 18:43
i believe that has to do with a bug in osx
Rick Medina
@rickmed
May 12 2016 18:43
ok
Ross Hinkley
@rosshinkley
May 12 2016 18:43
hang on, the issue is....
Rick Medina
@rickmed
May 12 2016 18:50
also, if there is a fatal error like a not existing selector, shouldn't the error be logged even if there is no catch?
Ross Hinkley
@rosshinkley
May 12 2016 19:22
sorry, got sidetracked
Rick Medina
@rickmed
May 12 2016 19:22
also, it's weird I don't see anywhere where there is an emitted event for 'log' on for the renderer object
Ross Hinkley
@rosshinkley
May 12 2016 19:22
i didn't spot the issue offhand
re the fatal error... probably?
not exactly sure where that would go
Rick Medina
@rickmed
May 12 2016 19:35
btw, I tested the listeners leak thing and it sets all involved listeners to 0 so :+1:
instead of log or error listenerscount going wild
Ross Hinkley
@rosshinkley
May 12 2016 20:03
by the by, the issue: segmentio/nightmare#434
for alwaysOnTop
Rick Medina
@rickmed
May 12 2016 20:12
thanks!
Ross Hinkley
@rosshinkley
May 12 2016 20:12
no problem :)
Rick Medina
@rickmed
May 12 2016 20:15
I researched a bit about the console thing but couldn't get to anything concrete
it is just sitting there collecting listeners for a not existant event
I think the original intent is to forward the logs of the window process console to the main nightmare console, so I changed it the the sent event "console" but nothing gets forwarded, just the log function starts to be called
so not sure what the original objective was...
Ross Hinkley
@rosshinkley
May 12 2016 20:17
you might want to remove it entirely
i could be mistaken, but the preload should take care of the console calls for you
Rick Medina
@rickmed
May 12 2016 20:27
I think so...
Ross Hinkley
@rosshinkley
May 12 2016 20:32
worth a test :)
Rick Medina
@rickmed
May 12 2016 21:09
I used several actions and it never gets called
also I did a code search and the event is never emitted anywhere
Ross Hinkley
@rosshinkley
May 12 2016 21:10
it may have gotten removed with some previous changes
Rick Medina
@rickmed
May 12 2016 21:10
but maybe there is a typo 'console' vs 'log' so it created a disconnect in a original functionality
Ross Hinkley
@rosshinkley
May 12 2016 21:11
it's also worth pointing out that maybe the log event never got updated
the console events were consolidated a while back
Rick Medina
@rickmed
May 12 2016 21:11
^^
Ross Hinkley
@rosshinkley
May 12 2016 21:11
(this was true - maybe still is? - in the javascript template)
Rick Medina
@rickmed
May 12 2016 21:12
yeap I looked into that also found weird the template doesn't call/uses the console
Ross Hinkley
@rosshinkley
May 12 2016 21:12
the template shouldn't
imho
Rick Medina
@rickmed
May 12 2016 21:12
no idea
Ross Hinkley
@rosshinkley
May 12 2016 21:13
the parts it wraps could use console methods, though
and those are wrapped in the preload
Rick Medina
@rickmed
May 12 2016 21:13
I have just got into the code from yesterday :)
Ross Hinkley
@rosshinkley
May 12 2016 21:13
(memory serving)
Rick Medina
@rickmed
May 12 2016 21:14
but yeah in general I think most likely a lost code in the versions update process
Rob Brackett
@Mr0grog
May 12 2016 21:14
@rickmed what is the missing logging you're talking about?
Ross Hinkley
@rosshinkley
May 12 2016 21:14
in the javascript handler, there's a log event handler
which logs if you have DEBUG=nightmare:log
Ross Hinkley
@rosshinkley
May 12 2016 21:15
where are the log events emitted, though?
console events are all wrapped up in the console family of events in the default preload
and i think (although i haven't gone back and looked) the log event used to be what handled console.log
Rob Brackett
@Mr0grog
May 12 2016 21:18
probably something someone broke in the distant past ¯\_(ツ)_/¯
I do believe that is what it used to do
Ross Hinkley
@rosshinkley
May 12 2016 21:18
there's log events child => parent
but not all client side
er, child side
i think it's vestigial
... it's not hurting anything at the moment, but it certainly isn't doing anything constructive :P
Rob Brackett
@Mr0grog
May 12 2016 21:20
broke here: segmentio/nightmare@6e4907f
Rick Medina
@rickmed
May 12 2016 21:20
vestigial, the learned word of the day :)
Rob Brackett
@Mr0grog
May 12 2016 21:21
never had a test, so I guess nobody caught it
rosshinkley @rosshinkley nods
Ross Hinkley
@rosshinkley
May 12 2016 21:22
tests, pff :P
Rob Brackett
@Mr0grog
May 12 2016 21:23
dunno if it's behavior worth reinstating
Ross Hinkley
@rosshinkley
May 12 2016 21:23
nnnno? won't console calls in evaluated code be handled by the ipc methods?
(i think there are console event tests, but you're making me doubt myself)
Rob Brackett
@Mr0grog
May 12 2016 21:25
nothing in the user process handles console events
that's for end-user usage only at current
Ross Hinkley
@rosshinkley
May 12 2016 21:26
are you talking about adding logging for internal methods' evaluate_now calls?
Rob Brackett
@Mr0grog
May 12 2016 21:26
uh
Ross Hinkley
@rosshinkley
May 12 2016 21:26
<== lost
Rob Brackett
@Mr0grog
May 12 2016 21:27
anything that evaluates, though I would think built-in code shouldn't need it
I think the meaningful value is:
I can write:
nightmare.evaluate(function() {
  doSomethingInWindow();
  console.log('blah blah'); // <-- this gets logged to actual system console
})
Obviously it doesn't do that now
but that's the only reason I can see for putting it back
it to get that kind of behavior
console.* calls in evaluate would actually go to stdout while console.* calls from web page content would not
(excepting if you had debug flags on)
or something
Ross Hinkley
@rosshinkley
May 12 2016 21:29
o_O
i'm following you... but i don't think nightmare behaved that way, piping console.log calls straight to the actual parent console
Rob Brackett
@Mr0grog
May 12 2016 21:30
no, it piped to the debug logger
Ross Hinkley
@rosshinkley
May 12 2016 21:30
yeah
Rob Brackett
@Mr0grog
May 12 2016 21:30
which, enh
Ross Hinkley
@rosshinkley
May 12 2016 21:30
and... should.. still...?
Rob Brackett
@Mr0grog
May 12 2016 21:30
whatevs, in my opinion
Ross Hinkley
@rosshinkley
May 12 2016 21:30
yeah
i see where you're coming from
it's reasonable to expect console methods to DWIM
Rob Brackett
@Mr0grog
May 12 2016 21:31
I think the current behavior is probably (?) just as fine as piping to the debug logger
maybe the debug logger should capture console logs regardless
that's a separate question, I think
Ross Hinkley
@rosshinkley
May 12 2016 21:31
iiii thought it did?
am i wrong?
Rob Brackett
@Mr0grog
May 12 2016 21:32
I don't think it does
Ross Hinkley
@rosshinkley
May 12 2016 21:32
mm.
dang.
Rob Brackett
@Mr0grog
May 12 2016 21:32
I don't think anything in the user process captures console events
anyway, I can see value in having console.* inside evaluate go to actual STDOUT/STDERR, but not sure if it's worthwhile value
but a thought worth throwing out there
Ross Hinkley
@rosshinkley
May 12 2016 21:33
for sure
... i'd say at a minimum, the output from console methods should be pumped into the debug logger
Rob Brackett
@Mr0grog
May 12 2016 21:33
in any case, shit done broke in 2.3.0, but it's shit that nobody done cared about
rosshinkley @rosshinkley chuckles.
Ross Hinkley
@rosshinkley
May 12 2016 21:34
if we were to alter that to pump console => console... is it worth adding an origin?
Rob Brackett
@Mr0grog
May 12 2016 21:34
like which browser window it came from?
Ross Hinkley
@rosshinkley
May 12 2016 21:34
i kind of feel like making .evaluate(() => console.log('hi')) log directly is ... misleading
Rob Brackett
@Mr0grog
May 12 2016 21:35
possibly
so screw it
Ross Hinkley
@rosshinkley
May 12 2016 21:35
hahahah.
tldr: make sure console.* logs to debug logger
fair?
Rob Brackett
@Mr0grog
May 12 2016 21:36
a) @rickmed should PR their patch for cleaning up those listeners
b) at the same time, we should just kill the console.log listener in the javascript action
c) push console events into debug logger
Ross Hinkley
@rosshinkley
May 12 2016 21:37
5- strongly agree on all counts
Rob Brackett
@Mr0grog
May 12 2016 21:56
hmmmm, I had never happened upon segmentio/nightmare#434
I can't reproduce; I wonder if it got fixed in newer Electrons
(would be nice not to have alwaysOnTop forced on)
Ross Hinkley
@rosshinkley
May 12 2016 21:59
mmmaybe?
you've got an osx box, yes?
i don't run with show:true terrifically often
Rob Brackett
@Mr0grog
May 12 2016 22:02
yeah, I mainly run OS X
ah, well
Ross Hinkley
@rosshinkley
May 12 2016 22:03
we could rip it out and see who complains ;)
capricious changes are one of the central tenets of earning HubCoin, isn't it? ;)
Rob Brackett
@Mr0grog
May 12 2016 22:05
hahaha
Rick Medina
@rickmed
May 12 2016 23:26
sorry guys had to go away for a while...
Rick Medina
@rickmed
May 12 2016 23:32
about the listeners leak thing, I'd love to do a PR for it but honestly, I haven't done a PR in my life (have just began learning js a few months ago) so I don't know how to do it let alone how to do tests for it, if you feel like it feel free to add the code yourselves...
Rob Brackett
@Mr0grog
May 12 2016 23:41
No better time to learn!
Start with just the code and we'll help you figure out the tests when you submit the PR
Rick Medina
@rickmed
May 12 2016 23:50
ok I'll give it a go
so what was the original intended behaviour with the console thing?
Rob Brackett
@Mr0grog
May 12 2016 23:53
We're not 100% sure, but the goal now it to just remove it :)
Rick Medina
@rickmed
May 12 2016 23:53
lol
just the 'log' listener?
Rob Brackett
@Mr0grog
May 12 2016 23:54
We think it was meant to log to the debug logger (so if you did $ DEBUG=nightmare:log node you-nightmare-script.js you'd see the logs from console.log)
But we're going to make that happen in a different (probably better) way now
Rick Medina
@rickmed
May 12 2016 23:54
nice
the logs from the instance (browser) console.log?
Rob Brackett
@Mr0grog
May 12 2016 23:55
yes
Rick Medina
@rickmed
May 12 2016 23:55
that's what I thought it was for initially, that is a useful feature to have
Rob Brackett
@Mr0grog
May 12 2016 23:56
you can already currently capture them by doing:
nightmareInstance
  .on('console', function(message) { console.log(message); })
  .goto('some-url')
  .evaluate(function() {
    // do some stuff
    console.log('some message');
  })
  .then(function() { something; });
Ross Hinkley
@rosshinkley
May 12 2016 23:58
sorry, i had to step away... +1 to submitting your first pr, we're here to help :)
Rick Medina
@rickmed
May 12 2016 23:59
I'm seeing a bunch of useful info in console of the instance when running scripts