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

tests: add page functions bundling test #15280

Merged
merged 5 commits into from
Jul 18, 2023
Merged

tests: add page functions bundling test #15280

merged 5 commits into from
Jul 18, 2023

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jul 17, 2023

These tests are a huge help in debugging issues with build-bundles wrt name mangling, which we work hard to avoid.

All of the minified variants of these tests currently fail if you switch buildBundle to use esbuild's minification.

Note: it is an official warning in the documentation that we shouldn't expect .toString to work for us :)

@connorjclark connorjclark requested a review from a team as a code owner July 17, 2023 23:22
@connorjclark connorjclark requested review from brendankenny and removed request for a team July 17, 2023 23:22
@connorjclark connorjclark changed the title test: add page functions bundling test tests: add page functions bundling test Jul 17, 2023
@@ -67,3 +67,4 @@ yarn.lock
**/*.d.cts
!**/types/**/*.d.ts

page-functions-test-case*out*.js
Copy link
Member

Choose a reason for hiding this comment

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

Some unit tests store random output files in the .tmp directory. WDYT about moving this output there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer where it is, since it is right next to the input file. Nicer for debugging.

build/test/.eslintrc.cjs Show resolved Hide resolved
@brendankenny
Copy link
Member

Does it make sense to take a wider perspective for a minute on the purpose of page functions? The fundamental goal is just to pass strings of javascript over the protocol to evaluate in the browser context. We created page-functions.js as a library of functions so each gatherer wouldn't have to reinvent stepping through the shadow dom, they could share the node format, etc.

In the service of that goal we keep adding layers to evaluate() in the pursuit of better DX, creating a pretty complex infrastructure with complicated implicit interactions across lighthouse (like with bundling), necessitating further infrastructure like these tests with their own bespoke mini-jsdom. All these layers do mean that more is taken care of automatically, but it also makes it harder to make a change at any level, debugging issues with the infrastructure becomes a full-time project for at least a few days at a time, etc.

Should we take a look at any possible simpler approaches to how we do page functions? There aren't that many uses of evaluate() if we wanted to switch things up.

@connorjclark
Copy link
Collaborator Author

Sure, it makes sense to see how we could redesign it all to make things like this not necessary.

As it is, this complexity already exists, so these tests just help wrangle it. And we must enable esbuild minification to un-stuck the build transition (we can't accept the regression in bundle size from #15239). So - land this and #15283 and evaluate some different approaches (prev discussion: #12771 (comment) #12795) for 11.0?

@adamraine
Copy link
Member

So - land this and #15283 and evaluate some different approaches (prev discussion: #12771 (comment) #12795) for 11.0?

This plan generally SGTM but I'm hesitant to expand the scope of 11.0 at this point. I wouldn't want it to be a release blocker. Also, is it necessarily a breaking change?

@connorjclark
Copy link
Collaborator Author

Also, is it necessarily a breaking change?

ExecutionContext is expose in our public gathering api, so it could be.

@connorjclark connorjclark merged commit a4f9313 into main Jul 18, 2023
@connorjclark connorjclark deleted the build-bundle-test branch July 18, 2023 22:57
@brendankenny
Copy link
Member

So - land this and #15283 and evaluate some different approaches (prev discussion: #12771 (comment) #12795) for 11.0?

FWIW #12795 did work with esbuild two years ago (all functions become local, so the relative re-naming doesn't matter), and esbuild hasn't changed that much since then.

But I'm also talking about even more fundamental reevaluating, going back to asking what exactly we need this to do, and what's the simplest way to accomplish that.

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.

4 participants