-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
V9 migrate from got to fetch #11942
V9 migrate from got to fetch #11942
Conversation
* (internal): drop support for Node.js 16 update engines of package.json files * (internal): drop support for Node.js 16 update CONTRIBUTING.md with Node.js 20 as recommendation * (internal): drop support for Node.js 16 update github workflows * (internal): drop support for Node.js 16 update external packages that dropped the support already * (internal): drop support for Node.js 16 resolve PR feedback * Update packages/webdriver/package.json * Update packages/wdio-cucumber-framework/package.json * Update package.json --------- Co-authored-by: Christian Bromann <[email protected]>
…iverio#11857) * (webdriverio): merge isDisplayed and isDisplayedWithinViewport * allow to specify withinViewport check in waitForDisplayed
* (internal): drop support for Node.js 16 update engines of package.json files * (internal): drop support for Node.js 16 update CONTRIBUTING.md with Node.js 20 as recommendation * (internal): drop support for Node.js 16 update github workflows * (internal): drop support for Node.js 16 update external packages that dropped the support already * (internal): drop support for Node.js 16 resolve PR feedback * Update packages/webdriver/package.json * Update packages/wdio-cucumber-framework/package.json * Update package.json --------- Co-authored-by: Christian Bromann <[email protected]>
…iverio#11857) * (webdriverio): merge isDisplayed and isDisplayedWithinViewport * allow to specify withinViewport check in waitForDisplayed
Progress UpdateTasks associated with this pull request.
Challenges and Blockers
|
Sounds reasonable to me. @erwinheitzman was working on #11542 to make this happen for all browser but it doesn't seem to be working. |
3d5670f
to
1d661ee
Compare
@tamil777selvan can we rebase this too? |
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.
Fantastic work 👏 really excited to clean up this mess of multiple request libs. Added some comments to further clean up the code, particularly the request factory is not needed anymore
|
||
export type Method = 'GET' | 'POST' | 'PUT' | 'PATCH' | 'HEAD' | 'DELETE' | 'OPTIONS' | 'TRACE' | 'get' | 'post' | 'put' | 'patch' | 'head' | 'delete' | 'options' | 'trace' | ||
|
||
export interface RequestLibOptions { | ||
agent?: Agents | ||
followRedirect?: boolean |
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.
Do we need RequestLibOptions
at this point? We can just rely on the fetch
types since we use it for all requests. I think we had this type to ensure that requests made through got
(in Node.js environments) and ky
(in browser environments) are compatible. We had this to support https://www.npmjs.com/package/web2driver.
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.
I guess we might need this. After extending RequestInit
, below are the additional options which are used internally. Please share your thoughts,
export interface RequestLibOptions extends RequestInit {
json?: Record<string, unknown>;
searchParams?: Record<string, string | number | boolean | null | undefined> | URLSearchParams;
timeout?: number;
url?: URL;
path?: string;
username?: string;
password?: string;
}
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 update to the RequestInit
interface everywhere, e.g.
json
: usebody: opts.body ?? JSON.stringify(opts.json)
searchParams
: useURLSearchParams
instead and add it to the urltimeout
: use AbortController as mentioned hereurl
remove as it is passed as second argumentpath
should not be needed anymore, I think it makes sense to have a helper function that can compose the full url based on connection credential optionsusername
andpassword
: useheaders.set('Authorization', 'Basic ' + btoa(username + ":" + password))
What do you think? Can we introduce these further cleanups? I am not sure how many times we pass along RequestLibOptions
but it would be idea if this is all using cross platform features.
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.
Certainly, the cleanup sounds logical. I'll get started on it.
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.
As suggested, I've made the cleanup on RequestLibOptions
. Regarding moving timeout
to AbortController
, I have not done because we're using it in the request.ts
file where we handle timeout clearance. I'm open to your thoughts on this.
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.
Can we get rid of it completely?
This is some amazing work! Well done 👏 |
headers: { | ||
Authorization: `Bearer ${token}`, | ||
} | ||
}) | ||
|
||
const { location } = headers | ||
const location = response.headers.get('location') |
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.
Is there ever a chance that location
is undefined?
const location = response.headers.get('location') | |
const location = response.headers.get('location') as string |
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.
So far, I've only observed the location as a string and not any other type.
|
||
export type Method = 'GET' | 'POST' | 'PUT' | 'PATCH' | 'HEAD' | 'DELETE' | 'OPTIONS' | 'TRACE' | 'get' | 'post' | 'put' | 'patch' | 'head' | 'delete' | 'options' | 'trace' | ||
|
||
export interface RequestLibOptions { | ||
agent?: Agents | ||
followRedirect?: boolean |
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.
Can we get rid of it completely?
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.
As @erwinheitzman pointed out, excellent work. Thanks a lot! This is a big milestone for v9!
* V9 drop support for Node.js 16 (#11493) * (internal): drop support for Node.js 16 update engines of package.json files * (internal): drop support for Node.js 16 update CONTRIBUTING.md with Node.js 20 as recommendation * (internal): drop support for Node.js 16 update github workflows * (internal): drop support for Node.js 16 update external packages that dropped the support already * (internal): drop support for Node.js 16 resolve PR feedback * Update packages/webdriver/package.json * Update packages/wdio-cucumber-framework/package.json * Update package.json --------- Co-authored-by: Christian Bromann <[email protected]> * (webdriverio): merge isDisplayed and isDisplayedWithinViewport (#11857) * (webdriverio): merge isDisplayed and isDisplayedWithinViewport * allow to specify withinViewport check in waitForDisplayed * V9 drop support for Node.js 16 (#11493) * (internal): drop support for Node.js 16 update engines of package.json files * (internal): drop support for Node.js 16 update CONTRIBUTING.md with Node.js 20 as recommendation * (internal): drop support for Node.js 16 update github workflows * (internal): drop support for Node.js 16 update external packages that dropped the support already * (internal): drop support for Node.js 16 resolve PR feedback * Update packages/webdriver/package.json * Update packages/wdio-cucumber-framework/package.json * Update package.json --------- Co-authored-by: Christian Bromann <[email protected]> * (webdriverio): merge isDisplayed and isDisplayedWithinViewport (#11857) * (webdriverio): merge isDisplayed and isDisplayedWithinViewport * allow to specify withinViewport check in waitForDisplayed * Migrating from got to fetch * Migrate got to fetch in packages * migrate got to fetch & update unit test * doc update for proxy setup * rebase and migrate got to fetch in browserstack * Migrate hostname from 0.0.0.0 to localhost * fix unit test by removing headers in safari driver * fix smoke test: update nock to beta * fix launch test for firefox * fix unit test for startGeckodriver * fix firefox executions in node 18 * Incorporating review comments * fix component tests * cleaning up RequestLibOptions * Removing RequestLibOptions & incorporating other review comments * use process from globalThis --------- Co-authored-by: Erwin Heitzman <[email protected]> Co-authored-by: Christian Bromann <[email protected]>
Proposed changes
Fixes #11862
Types of changes
Checklist
Further comments
Reviewers: @webdriverio/project-committers