fix: older browsers support in time travel and assertView#1281
Conversation
✅ Testplane browser-env run succeed
|
commit: |
✅ Testplane E2E run succeed
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f540130eef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let debugBrowserId = ""; | ||
| if (debug.enabled) { | ||
| debugBrowserId = `${(browser as WdioBrowser)?.capabilities?.browserName} ${ |
There was a problem hiding this comment.
этого точно хватит для id? у нас же может быть 2 браузера одинаковой версии, но с разными капабилити (desktp/touch)
| // @ts-expect-error | ||
| return Boolean(window.rrweb); | ||
| } catch { | ||
| } catch (e) { |
There was a problem hiding this comment.
The catch {} syntax became available from chrome 66, while we are targeting chrome 53: https://caniuse.com/wf-optional-catch-binding
| } | ||
|
|
||
| if (bundlesCache[scriptFilePath]) { | ||
| debug( |
There was a problem hiding this comment.
Why would we need to know it?
This debug log does not seem to be usefull
There was a problem hiding this comment.
It actually is very useful, for example it highlights which version of the script — native or compat was chosen, which instantly makes it clear if calibration works as expected, etc.
| let result = await collectRrwebEvents(session, shouldSendRrwebCode ? rrwebCode : undefined); | ||
|
|
||
| if (result.isRrwebSupported === false) { | ||
| debug("rrweb is not supported in this browser, error: %s", result.evalError); |
There was a problem hiding this comment.
this browser
Maybe we should specify, what browser are we talking about? Like name and version
| const colorSchemeMedia = rrwebData && rrwebData.colorSchemeMedia; | ||
| const colorSchemeListener = rrwebData && rrwebData.colorSchemeListener; |
There was a problem hiding this comment.
optional chaining is supported from chrome 80: https://caniuse.com/mdn-javascript_operators_optional_chaining
While we are targeting much earlier versions.
| function collectRrwebEvents( | ||
| session: WebdriverIO.Browser, | ||
| rrwebRecordFnCode: string | null, | ||
| rrwebRecordFnCode: string | undefined, |
There was a problem hiding this comment.
nit: -> rrwebRecordFnCode?: string
There was a problem hiding this comment.
Well it's actually important here.
The story is that in older versions, argument is expected to be string or undefined precisely (with null it throws an error). However, we don't mean that "argument can be omitted". We mean precisely that it must be passed, but can be either string or undefined.
…e to support older browsers
3 participants
What's done?
All these changes tested on Chrome 53
Not tested on internet explorer, because it turns out it was broken a long time ago:
testplane/src/browser/new-browser.ts
Line 223 in f540130