Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.use usage #661

Closed
LinusU opened this issue May 26, 2016 · 9 comments
Closed

.use usage #661

LinusU opened this issue May 26, 2016 · 9 comments

Comments

@LinusU
Copy link
Contributor

LinusU commented May 26, 2016

What is the intended use case for .use(fn)?

I had hoped that it could be used to add actions to nightmare instances, but I haven't gotten that to work since it seems that I can only add actions on the global Nightmare class.

Maybe I'm just missing something...

@rosshinkley
Copy link
Contributor

What is the intended use case for .use(fn)?

.use() is intended to wrap a group of actions to make adding compound actions to Nightmare a little easier and readable. Note that it doesn't add callable actions directly to the instance itself. (In other words, you can't call nightmare.myAction(...), but you can call nightmare.use(myAction(...)).)

I had hoped that it could be used to add actions to nightmare instances, but I haven't gotten that to work since it seems that I can only add actions on the global Nightmare class.

I am guessing you're referring to .action()? This is a very different mechanism - .action() adds an action directly to the Nightmare prototype, also adding all of the sugar required to execute a method inside of Electron.

What are you trying to do?

@LinusU
Copy link
Contributor Author

LinusU commented May 26, 2016

I'm trying to build a module for dealing with <select> elements, and I want an easy way for other people to consume the library. At first, I thought that .action was an instance method and thus I wrote my readme as this:

## Usage

```js
const Nightmare = require('nightmare')
const NightmareSelect = require('nightmare-select')

const nightmare = new Nightmare()

nightmare.use(NightmareSelect)

await nightmare.goto('http://...')

if (await nightmare.selectHasOption('select.color', 'blue')) {
  await nightmare.selectOption('select.color', 'blue')
}

if (await nightmare.selectHasOptionWithText(select.color, 'Blue color')) {
  await nightmare.selectOptionWithText('select.color', 'Blue color')
}
```

I don't like the idea of encouraging my users to patch the global Nightmare object, as that is considered a bad practice and could potentially have side effects on other libraries.

@rosshinkley
Copy link
Contributor

In the development of .action(), I had originally thought about adding functionality per instance, but this was abandoned for simplicity's sake. Is allowing a type to modify it's own prototype bad practice? (Debatable.)

I think .action() is probably better suited to your use-case. It shouldn't step on other libraries so long as there aren't naming collisions. Doing what you're asking with .use() would probably require calling .then(), which might have undesirable effects.

Thinking out loud: would .exists() and .select() accomplish what you're after?

@rosshinkley
Copy link
Contributor

For what it's worth: I've thrown together a .action() and .use() examples over at nightmare-examples.

@LinusU
Copy link
Contributor Author

LinusU commented May 31, 2016

Nice! That helped me understand what .use was intended for.

I don't know if this should be encouraged at all but I've been extending the prototype manually so that I can get support for plugins in a nice way, not sure if I want to release anything using this though, but this is what it looks like:

nightmare-select.js

function selectHasOption (selector, value) {
  return this.evaluate((selector, value) => {
    // ...
  }, selector, value)
}

function selectHasOptionWithText (selector, text) {
  return this.evaluate((selector, text) => {
    // ...
  }, selector, text)
}

// ...

module.exports = function (nightmare) {
  nightmare.prototype.selectHasOption = selectHasOption
  nightmare.prototype.selectHasOptionWithText = selectHasOptionWithText
  // ...
}

usage.js

const Nightmare = require('nightmare')
const NightmareSelect = require('nightmare-select')

const nightmare = new Nightmare()

nightmare.use(NightmareSelect)

await nightmare.goto('http://...')

if (await nightmare.selectHasOption('select.color', 'blue')) {
  await nightmare.selectOption('select.color', 'blue')
}

if (await nightmare.selectHasOptionWithText(select.color, 'Blue color')) {
  await nightmare.selectOptionWithText('select.color', 'Blue color')
}

This way I won't ever clobber the global instance which I really want to avoid...

@rosshinkley
Copy link
Contributor

rosshinkley commented May 31, 2016

That's really what .action() was intended for. You could rewrite your selectHasOption and selectOptionWithText as .action()s. From the hip:

Nightmare.action('selectHasOption', (selector, option, done) => {
  return this.evaluate_now((selector, option) => {
    // ...
  }, done, selector, option);
});

Nightmare.action('selectOptionWithText', (selector, option, done) => {
  return this.evaluate_now((selector, option) => {
    // ...
  }, done, selector, option);
});

Under the covers, Nightmare amends the prototype with the action you specify.

@LinusU
Copy link
Contributor Author

LinusU commented May 31, 2016

Yes, but the problem with that I don't see a way of using that without going down a path of anti-patterns. The use case here is that I want to distribute a third party "plugin" to nightmare.

The first problem with the action approach, is 1) modifying global state potentially affecting other parts of the code, and 2) require with side effects.

require('nightmare-select') // Nightmare is now magically patched
const Nightmare = require('nightmare')

// ...

So I refactor a bit to avoid the require with side effects. Instead of running on load, I'll export a function that will patch the nightmare object.

const Nightmare = require('nightmare')
const NightmareSelect = require('nightmare-select')

NightmareSelect.patch(Nightmare)

// ...

But the bigger problem still exists, I'm modifying global state. Lets say that I have several scrapers for different sites published on npm. Two of them, scrape-bikes-stockholm and scrape-bikes-uppsala depends on two different versions of nightmare-select. Since I made a breaking change in the api, I had to bump the version to 2.0.0. I have updated scrape-bikes-stockholm, but I haven't gotten around to updating scrape-bikes-uppsala just yet.

Now there will be a huge conflict in my app, bikes-in-sweden, which shows the position of all bike racks in Sweden. Since it depends on both scrape-bikes-stockholm and scrape-bikes-uppsala, it will pull in both versions of nightmare-select. But when the second to load version is loaded, it will simply replace the first version and the scraper depending on that version will start to fail.

The Node.js community is very centered on small reusable modules, and I think that publishing a lot of small modules is not an uncommon use case. Encouraging users to patch global state is dangerous and considered an anti-pattern.

I would be very happy if we could see if there are any alternative approaches to a great plugin system, that is both easy to use, reliable and compatible with the Node.js ecosystem 🙌

also, Nightmare is really awesome and it beats all the other similar tools I've used, keep up the good work!

@rosshinkley
Copy link
Contributor

First and foremost: I agree. Modifying the prototype directly for actions has problems.

The idea behind modifying the prototype was, again, done for brevity/cleanliness and so you're not slogging through a ton of boilerplate just to get a Nightmare instance. Furthermore, patching the prototype is how internal methods are attached. Allowing users to leverage that was a reasonable outgrowth. Also, patching of Electron behavior was built atop the already-existing patching of Nightmare behavior.

I'm not saying it's correct. Scope and use of Nightmare has changed. A lot.

This was sort-of-kind-of touched upon in this comment. I think better plugin/action management should be put on the v3 list in #593. Thoughts?

@rosshinkley
Copy link
Contributor

Merging this with #593.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants