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: make main tsc compile cacheable #13069

Merged
merged 1 commit into from
Sep 15, 2021
Merged

core: make main tsc compile cacheable #13069

merged 1 commit into from
Sep 15, 2021

Conversation

brendankenny
Copy link
Member

The main typescript compilation (from the root tsconfig.json) hasn't had emitDeclarationOnly turned on because of compilation errors I ran into in #12914 when I initially enabled that. Not having declaration emit was ok, though, because it's a compilation root, not a dependency for any of our other tsconfigs, so it didn't need a d.ts emit for anything. The downside is that less of it is cacheable and more has to be re-done on each compile, even if nothing in the core files has changed.

I ran into this problem again in a different context and it turns out it's fairly simple, only a single error (I thought it was the first of many), and it even tells you exactly what you need to do to fix it.

Declaration emit for this file requires using private name 'NetworkRecords' from module '"lighthouse/lighthouse-core/computed/network-records"'. An explicit type annotation may unblock declaration emit.

This allows fully caching the core compile, so if e.g. you're working on report stuff, you'll only spend ~100ms instead of 5s recompiling core files.

@brendankenny brendankenny requested a review from a team as a code owner September 15, 2021 21:01
@brendankenny brendankenny requested review from connorjclark and removed request for a team September 15, 2021 21:01
@google-cla google-cla bot added the cla: yes label Sep 15, 2021
Comment on lines +74 to +76
// Explicit type reference (hidden by makeComputedArtifact) for d.ts export.
// TODO(esmodules): should be a workaround for module.export and can be removed when in esm.
/** @type {typeof import('./computed/network-records.js')} */
Copy link
Member Author

Choose a reason for hiding this comment

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

kind of a dumb workaround :) The need for it is kind of covered by microsoft/TypeScript#37832 (comment) and microsoft/TypeScript#41672, but it's obviously not fixed and it's not a typedef causing the issue.

tl;dr: typescript creates an (ESM-ish) export object for the d.ts file, but needs a type for NetworkRecords. It then gets confused by the type because NetworkRecords is a computed artifact, which subsumes the original NetworkRecords type, but the original NetworkRecords is still needed because the computed artifact return type is NetworkRecords & {request(artifacts: DevTools, cache: LH.Artifacts.ComputedContext): Promise<whatever>}, hence the "requires using private name 'NetworkRecords'" complaint.

"Kind of dumb" because the annotation is exactly the expected type for the require(), but just need to explicitly type it to unconfuse the compiler and it should go aways with the conversion to es modules anyways, so 🤷

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.

3 participants