Skip to content

Conversation

@aberonni
Copy link
Contributor

This is an attempt at closing #117.

As discussed in the comments of that issue, chromedriver.start() now returns a promise that resolves when chromedriver is ready and listening for connections. It resolves with the child process that was previously what chromedriver.start() returned right away.

This is a breaking change, seeing as the function previously returned the child process right away, so it should probably be released as a major release.

@giggio
Copy link
Owner

giggio commented Oct 24, 2018

I just reviewed it and tested it and it looks good.

The only problem is the breaking change. As we've been following Chromedriver's versions, at least on major and minor, I can't wait for a major release, or we will only release this when Chromedriver v3 comes out, and we have no idea of when that might be.

I see two options to maintain compatibility:

  • We can create another function for people who want the promise, and deprecate start with a console warning and jsdoc. And then, an year from now we remove start.
  • We can add an extra argument to start and only return the promise when we get it.

What do you think?

@aberonni
Copy link
Contributor Author

@giggio I understand the major release issue, I hadn't considered that.

I don't have a strong preference between the two, but maybe I am slightly leaning towards an extra argument. I'll try updating the PR to reflect this and let you know.

chromedriver.start() now returns a promise that resolves with the
child process once the chromedriver is ready and listening for
connections

this is a breaking change, seeing as the function previously
returned the child process itself, so it should probably be released as
a major release
@aberonni
Copy link
Contributor Author

@giggio I updated the PR, let me know what you think

@giggio
Copy link
Owner

giggio commented Oct 26, 2018

LGTM, it is not beautiful, but it is compatible. Thanks!

@giggio giggio merged commit 20e153e into giggio:master Oct 26, 2018
Janpot added a commit to Janpot/next.js that referenced this pull request Feb 24, 2019
chromedriver can do it on its own now
giggio/node-chromedriver#177
@lock
Copy link

lock bot commented Jul 10, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants