-
Notifications
You must be signed in to change notification settings - Fork 334
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
Update Puppeteer render()
to bypass Review app
#4345
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 9f3ab07 |
f34d290
to
11e07aa
Compare
5efc072
to
6135afd
Compare
11e07aa
to
629872d
Compare
6135afd
to
6169562
Compare
629872d
to
326f090
Compare
6169562
to
c9c75cc
Compare
c9c75cc
to
ebc1b99
Compare
326f090
to
c2cc1ae
Compare
ebc1b99
to
869ade3
Compare
869ade3
to
ec012af
Compare
ec012af
to
08787b3
Compare
08787b3
to
510437b
Compare
510437b
to
ec5c574
Compare
ec5c574
to
0475eaf
Compare
0ebeb59
to
9187532
Compare
40d5d26
to
6282e07
Compare
6282e07
to
c803301
Compare
c803301
to
927a729
Compare
9187532
to
30baef5
Compare
927a729
to
eb70c22
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.
That's a neat step towards adressing #2565 🥳 Overall looks good and I think I followed OK, just want to confirm why a few things are happening. Don't think there'll be any blocker for the answers, but feels useful to have a trace of why things changed in a couple of places 😊
By passing a base to `new URL()` we can switch between the Review app and `file://` URLs
This prevents page and asset requests from hitting the Review app entirely
Since fonts are downloaded asynchronously, we should await the `document.fonts.ready` promise since further network requests may not be available once the page has loads Similarly, some components are made visible after initialising JavaScript (or new elements created and inserted) so we should wait for additional font downloads to complete there too E.g. GDS Transport Light may have loaded already, but GDS Transport Bold may be requested on demand as required
eb70c22
to
2a29ce8
Compare
// Mock preview HTTP response | ||
page.once('request', (request) => | ||
request.respond({ | ||
body: renderPreview(componentName, renderOptions), | ||
contentType: 'text/html' | ||
}) |
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.
Nice one, that makes things tidier for the handler serving the assets 😊
Part of #4103
Updates our
render()
test helper from Puppeteer (previouslyrenderAndInitialise()
) so it doesn’t send a request to the Review app, but instead runs against local preview of the component.This is a first step towards restoring tests failing when components throw by having them not run against review app pages (which run
initAll()
, which logs errors)By initialising individual components we ensure that future errors cause tests to fail