segment-boneyard / nightmare

A high-level browser automation library.
https://open.segment.com
19.55k stars 1.08k forks source link

v3 features #593

Open rosshinkley opened 8 years ago

rosshinkley commented 8 years ago

I'm not sure where to put this, and I'm open to this being moved to a Wiki if that makes more sense. In the interest of having all of the big (and possibly breaking) features/nice-to-haves for the next big release in one spot:

There may be a need for:

Nice to-haves:

What have I missed in features, needs, or linked issues?

edit: added element

Mr0grog commented 8 years ago

596 makes me wonder if changing viewport() to actually set the viewport (as opposed to window) size would be a good breaking change for v3.

Mr0grog commented 8 years ago

Maybe have goto return as soon as a page is loading so you can execute scripts ASAP during load?

Have actions with selectors wait for the selector before doing the action (#388). Capybara (Ruby), for example, does this. It could possibly be configurable (though I think you’d want opt-out rather than opt-in).

rosshinkley commented 8 years ago

IPC safety will likely make it into the next release of Nightmare.

rosshinkley commented 8 years ago

596 makes me wonder if changing viewport() to actually set the viewport (as opposed to window) size would be a good breaking change for v3.

Hm, how are you planning on doing that? Using window.outerWidth and outerHeight and then adding the difference? (I don't remember being able to set the viewport directly, but I'm probably wrong.)

Maybe have goto return as soon as a page is loading so you can execute scripts ASAP during load?

To what end? Should preload be extended (or pluggable) instead?

Have actions with selectors wait for the selector before doing the action (#388). Capybara (Ruby), for example, does this. It could possibly be configurable (though I think you’d want opt-out rather than opt-in).

Fully on-board for an implementation of #388.

Mr0grog commented 8 years ago

how are you planning on [changing the viewport size instead of window size]?

I think just calling window.setContentSize() instead of window.setSize()

To what end? Should preload be extended (or pluggable) instead?

I think I was thinking so you could get in as early as DOMContentLoaded or maybe before other scripts, but maybe specifying preload stuff somehow would be better. Not an idea I’d thought through deeply.

rosshinkley commented 8 years ago

I think just calling window.setContentSize() instead of window.setSize()

Of course. Y'know, I've read the browser-window docs a zillion times and forgot about setContentSize. Maybe I need to read it a few more times.

I think I was thinking so you could get in as early as DOMContentLoaded or maybe before other scripts, but maybe specifying preload stuff somehow would be better.

I was pondering out loud, and now I'm wondering if plugging preload is a good idea. I think preload is executed prior to every page load, in so doing losing a bit of control - there are probably situations where you want to run a given script on a given page's load.

I doubt I'm thinking about it the way you are: what would you suggest as API changes? It seems like you're advocating having a navigation setup method and a navigation method - am I anywhere close to being on the same page?

rosshinkley commented 8 years ago

I've been tinkering with the idea of changing the Electron part of .action() to wrap the ambient require. The function signature is ugly: function(name, options, parent, win, renderer, done). Changing the .call in the action handler from:

.call({require: require, parent: parent})

...to something more like....

.call({
  require: (lib) => {
    if(lib==='nightmare-plugin') {
      return {
        name: name,
        options: options,
        parent: parent,
        win: win
      };
    }
    return require(lib);
  },
  parent: parent
})

... which would make the handler signature be function(done). Also, dropping the renderer as I think it could be required.

I realize wrapping require is also kind of ugly and probably unadvised, but I thought I'd float it out. Thoughts?

Mr0grog commented 8 years ago

I definitely agree that function signature is not only a little ugly, but very hard to get right. I have had to look it up in the README literally every single time I’ve needed to use it and I’ve gone so far as to write it like function (_, __, parent, ___, ____, done) in some examples because you really only need one or two of those most of the time.

But why not just make those variables ambiently available in the plugin’s closure instead of wrapping require? Or, barring that, have plugins take two args: an object with all the things (as you’d get from the require call in your example) and the done callback?

(Also agree that dropping renderer would be pretty reasonable because there are other ways to get it.)

Mr0grog commented 8 years ago

I was pondering out loud, and now I'm wondering if plugging preload is a good idea.

It’s probably not. I think my real desire, initially, was just to be able to run code before waiting for the whole page to fully finish loading all its resources (exactly how, in the client, you often want to run on DOMContentLoaded instead of on load). My use case is basically: “I want to click this button and I don’t care whether images have loaded yet.”

BUT the more I think about it, that makes a lot of things more complicated: have all the page’s scripts (if some where deferred) loaded yet? What if you just want to load a page and screenshot it? Now you have to load, wait, then screenshot. Etc. A possible alternative might just be to add an option to goto asking it to return as soon as the HTML is loaded, but keep the default behavior the same. (e.g. goto('url', {waitForResources: false}))

rosshinkley commented 8 years ago

I have had to look it up in the README literally every single time I’ve needed to use it

Me too. And I wrote the [expletive redacted] thing. I'm going to go on a limb and say it's the most unwieldy part of Nightmare, and humbly apologize.

But why not just make those variables ambiently available in the plugin’s closure instead of wrapping require?

It's certainly easy to do, but makes where those variables came from kind of opaque. The less it seems like magic, the better. (Using require only makes this marginally better, I realize.)

Or, barring that, have plugins take two args: an object with all the things ...

The options hash was frowned upon in the orginal incarnation. I wouldn't be totally against it (provided it has a descriptive name), but require seemed to satisfy both the "no options hash" and the "this function signature is impossible to remember" demi-requirements.

rosshinkley commented 8 years ago

My use case is basically: “I want to click this button and I don’t care whether images have loaded yet.”

That's what I was guessing, and that would be awkward to cram into preload. That's why I walked it back. I'm not sold that plugging preload is a bad thing necessarily (having to redefine all of the machinery for console et al is not a great solution, either), but not going to solve what you had in mind.

BUT the more I think about it, that makes a lot of things more complicated...

This is what I was thinking, but I thought maybe there was an obvious part that I had missed. (I have a penchant for overcomplication.)

A possible alternative might just be to add an option to goto asking it to return as soon as the HTML is loaded, but keep the default behavior the same. (e.g. goto('url', {waitForResources: false}))

I'm trying to think of countercases, and while I can think of instances where not waiting is silly, they are still valid use cases and I think would behave properly. (Eg, .goto(myUrl, {waitForResources: false}).wait('.mySelector') where .mySelector is added by the client.) At first blush, this is a reasonable approach.

Mr0grog commented 8 years ago

Generally: have every action return something? In a similar vein, be better about errors. Also: make all errors instances of Error (maybe a subtype, like NightmareError or something). This covers a lot of things, so probably requires an audit of all existing actions first (if it seems like a good idea).

rosshinkley commented 8 years ago

Generally: have every action return something? In a similar vein, be better about errors.

Can't +1 this enough.

maybe a subtype, like NightmareError or something

That's something I had not considered, but the idea is certainly appealing.

so probably requires an audit of all existing actions first (if it seems like a good idea)

This is probably worth doing. Added #646.

overflowz commented 7 years ago

Seriously, when this release will be available? Popups / New windows are giving me serious nightmares and struggling a lot 😭 I'm still using nightmare-window-manager, but sometimes it does not help (popup logins for example, triggered by javascript with anti csrf token + link of something, etc and all of these are obfuscated or really painful to dig in).

rosshinkley commented 7 years ago

@overflowz The main holdup right now is that preload is not honored by windows not created by instantiating a new BrowserWindow internal to Electron, specifically with window.open(). See electron/electron#2605.

I haven't dug into it, but it might be skirted with some preload magic on the main Electron window. At the time, it felt like an awful lot of work for something that might end up getting fixed.

TimNZ commented 7 years ago

Does handling webContents 'new-window' event solve the preload issue?

https://electron.atom.io/docs/api/web-contents/#event-new-window