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

Export Runner, not just its type. #4227

Merged
merged 1 commit into from
Apr 14, 2017
Merged

Conversation

LucasSloan
Copy link
Contributor

No description provided.

Copy link
Contributor

@NickTomlin NickTomlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense and opens up some different uses for runner.

An aside for my curiosity: what are you using this for? Protractor has had some requests for programmatic usage examples and improvements. If you have an open source example that may be of great boon to the docs.

@vikerman vikerman merged commit 8249167 into angular:master Apr 14, 2017
@vikerman
Copy link
Contributor

This was for a very specific use where we had to set the spec name in the browser capabilities and hence we wanted to override the implementation of createBrowser.

@sjelin
Copy link
Contributor

sjelin commented Apr 14, 2017

I do not like this. The runner is not a public API and we shouldn't be encouraging people to hack it like this. If people are determined to do this kind of hack, they can use require('protractor/built/runner.js').Runner;.

The long term solution to @vikerman's problem is to have a plugin hook in createBrowser that allows people to modify the capabilities (#4230).

@heathkit
Copy link
Contributor

@sjelin agreed, I think it would be better to keep runner private.

@NickTomlin
Copy link
Contributor

I think I misunderstood the benefits here; my bad :(. I believe @sjelin and @heathkit are right on this. I love the idea of exposing more "building blocks" to external users but it would be best to do that with a crafted external API and not by just exposing the internals (as they stand) to the outside world.

@vikerman can we revert this and go with a require based reach into the internals as a short term solution while we eventually move towards the solution in #4230 ?

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

Successfully merging this pull request may close these issues.

None yet

6 participants