baudehlo / node-phantom-simple

Simple bridge to phantomjs for Node
MIT License
201 stars 70 forks source link

Create parameter to enable jsconsole for SlimerJS. #112

Closed KevinGrandon closed 8 years ago

KevinGrandon commented 8 years ago

@baudehlo - Thanks for the great library here! I've started using it for GhostJS, and so far have found this really useful.

One thing that I could not figure out how to do was to launch the SlimerJS jsconsole. It seemed that it wasn't compatible with the current parameter handling. This patch is just one way of addressing it, but please let me know if you'd like to approach it in a different way. Thanks!

puzrin commented 8 years ago

I don't like idea to mix browser options into main ones. If something don't allow to pass options in standard way, problem should be solved there.

puzrin commented 8 years ago

At first glance, it's possible to pass option names started with - as is. That will not conflict with existing logic.

KevinGrandon commented 8 years ago

At first glance, it's possible to pass option names started with - as is. That will not conflict with existing logic.

Hi, thanks for the help. Could you tell me how? I would love to get this documented and in the README if possible.

It is possible to open the jsconsole with something like this, but then I get an error running the test:

parameters: {'jsconsole': true}

Error received:

HeadlessError: Unexpected output from PhantomJS: Error: script not found

I believe this is because it tries to generate the parameter like --jsconsole=true.

If something don't allow to pass options in standard way, problem should be solved there.

I believe that would be a gecko patch, and one that would be non-trivial to write.

puzrin commented 8 years ago

I mean, code change required anyway, but could be more gently. Now driver always add -- to option name and "dashify" camelcased ones. But it could pass option names, started with - as is.

KevinGrandon commented 8 years ago

Thanks for the clarification, happy to do that.

puzrin commented 8 years ago

Note, my comment was not final, only a quick example that things can be done better. Make sure that all cases will be covered https://docs.slimerjs.org/current/configuration.html. It seems slimer has more variation.

puzrin commented 8 years ago
-jsconsole
-P name
-CreateProfile name
-profile path

All those slimer options can not be passed now.

KevinGrandon commented 8 years ago

@puzrin I've added a few commits here (happy to squash them if you're happy with the result).

I could not find a nice way to solve all of the cases using a javascript object. I think ideally you might want to consider following what node exec does for arguments, as it's a well known pattern. I've added support for the parameters object to be both an object and an array. I think this is probably the most flexible option for now, but please let me know what you think.

puzrin commented 8 years ago

May be { "-P": [ "foo", "bar" ] } => (-P foo bar)? Shitty, but compatible.

+ alternative to pass array as options object.

puzrin commented 8 years ago

Let's look from another end. It would be nice to have non-magical way to pass any arguments to browser. Options as Array or raw args (like in node spawn) is ok. We will have to support this format anyway.

If that's enougth for for your needs, i would add node-like array support and leave all magic for future (that will never come :).

KevinGrandon commented 8 years ago

@puzrin Thanks for the help.

I think that landing this initial array support is the way to go and it solves my use case. I'd suggest landing this for now, and we can look at doing something more magical in the future.

I've squashed these commits, so whenever you can land and increment the version I would appreciate it. Thanks!

puzrin commented 8 years ago

Thanks! Published.