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

core(replay): @puppeteer/replay stringify extension #14146

Merged
merged 29 commits into from
Aug 16, 2022
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Jun 22, 2022

🔒 Doc

@puppeteer/replay stringify will take a user flow JSON as input, and output a standalone Puppeteer script. We can use the stringify extension api to inject Lighthouse user flow code into the output script.

The intent is for this to be rolled into DevTools, and used to add an "Export as Puppeteer script w/ Lighthouse" option to the list of export options. We could also support users importing this extension themselves, but that is not 100% necessary at this time.

package.json Outdated Show resolved Hide resolved
@adamraine adamraine changed the title WIP: stringify user flow replay JSON core(replay): @puppeteer/replay stringify extension Jul 20, 2022
@adamraine adamraine marked this pull request as ready for review July 20, 2022 22:35
@adamraine adamraine requested a review from a team as a code owner July 20, 2022 22:35
@adamraine adamraine requested review from connorjclark and removed request for a team July 20, 2022 22:35
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Can you add a description of what a stringify extension is and how this is expected to be used? Are people going to call this code? DT integration?

@@ -46,6 +46,7 @@ jobs:

- run: yarn install --frozen-lockfile --network-timeout 1000000
- run: yarn build-report
- run: yarn reset-link
Copy link
Member

Choose a reason for hiding this comment

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

is this required to run unit tests after this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@@ -0,0 +1,102 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

what are you thinking about updating these as the replay library changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO the JSON is small enough where we can make changes manually.

package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,485 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Running the stringified output script generates a valid desktop flow report 1`] = `
Copy link
Member

Choose a reason for hiding this comment

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

seems like a lot of this is snapshotting @puppeteer/replay code

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeaaaaah...

The unit test file does some trimming to get rid of a lot of the boilerplait stuff. However, I wanted some tests to validate the entire output though, just so we know what @puppeteer/replay changes when we bump versions.

@@ -7612,6 +7640,19 @@ yargs@16.2.0, yargs@^16.1.1, yargs@^16.2.0:
y18n "^5.0.5"
yargs-parser "^20.2.2"

yargs@17.5.1:
Copy link
Collaborator

@connorjclark connorjclark Aug 16, 2022

Choose a reason for hiding this comment

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

unfortunate that we are now adding another yargs + all this other CLI color crap to our deps with this PR.

At the very least, we could see about upgrading to latest yargs to reduce this a bit. I'll try this now.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

I recently made changes to Server (static-server), so should update from master first.

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

Successfully merging this pull request may close these issues.

5 participants