-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat(init): ***breaking change*** pass webdriver instance into init()
…
#83
Conversation
init()
…
dc58c3f
to
3876a16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to have a before/after code block in the commit message (I think just copying the require.init block from the readme would work)
usingServer('http://localhost:4444/wd/hub'). | ||
withCapabilities({browserName: 'chrome'}). | ||
build(); | ||
require('jasminewd2').init(webdriver.promise.controlFlow(), webdriver); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably still show this webdriver step, since we use the global driver
below.
@@ -12,16 +12,15 @@ | |||
], | |||
"author": "Julie Ralph <ju.ralph@gmail.com>", | |||
"dependencies": { | |||
"jasmine": "2.4.1", | |||
"selenium-webdriver": "3.0.1" | |||
"jasmine": "2.4.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never actually require('jasmine')
in index.js
, so we could make this a devDependency
. I'm not sure explicitly depending on jasmine
actually does anything for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I think that's a fair point, it only enabled conflicts having it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@juliemr all comments addressed |
…)` instead of using `require()` So where as before you would write: ```js require('jasminewd').init(webdriver.promise.controlFlow()); ``` Now you will write: ```js require('jasminewd').init(webdriver.promise.controlFlow(), webdriver); ``` This removes the dependency on `selenium-webdriver` and protects jasminewd from having a different webdriver instance than Protractor, which could be a huge problem if they had different control flow settings. This is a breaking change because it changes the API for the `init` function. I also removed the dependency on jasmine, which didn't do anything anyway. Maybe it should have been a peerDependency but those are deprecated.
… instead of using
require()
This removes the dependency on
selenium-webdriver
and protects jasminewd from having a differentwebdriver instance than Protractor, which could be a huge problem if they had different control flow
settings.
This is a breaking change because it changes the API for the
init
function.