-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore: prepare to move recorder app to the client side #36973
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
base: main
Are you sure you want to change the base?
chore: prepare to move recorder app to the client side #36973
Conversation
dgozman
commented
Aug 9, 2025
- Move codegen to isomorphic, generate code on the client side for the recorder api.
- Move deviceDescriptors to isomorphic, as these are used by codegen.
- Remove ProgrammaticRecorderApp as it does not do anything anymore.
- Move actionUpdated logic to the recorder core.
Test results for "tests 1"6 failed 43 flaky46600 passed, 805 skipped Merge workflow run. |
}; | ||
const languageGenerator = languages.find(l => l.id === params.language) ?? languages.find(l => l.id === 'playwright-test')!; | ||
|
||
this._channel.on('recorderEvent', ({ event, data, page }) => { |
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.
How is enable/disable/enable scenario supported?
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.
Good point, updated.
|
||
static async show(context: BrowserContext, params: channels.BrowserContextEnableRecorderParams) { | ||
if (process.env.PW_CODEGEN_NO_INSPECTOR) | ||
if (process.env.PW_CODEGEN_NO_INSPECTOR || params.recorderMode === 'api') |
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.
recorderMode === 'api' should not get here?
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 does not. I guess I can replace the check with the comment, that would be better.
if (debugMode() === 'inspector') | ||
await RecorderApp.show(this, { pauseOnNextStatement: true }); | ||
|
||
// When paused, show inspector. |
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.
Where did this go?
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.
This was dead code. We are initializing the browser context, cannot be already paused here.
* limitations under the License. | ||
*/ | ||
|
||
import { CSharpLanguageGenerator } from './csharp'; |
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.
Any reason move is not detected?
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 adding types to this small file pushed it over the "changed" threshold.
12df65c
to
f8ce4b0
Compare