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(tsc): add base tsconfig for config inheritance #13072

Merged
merged 3 commits into from
Sep 16, 2021
Merged

Conversation

brendankenny
Copy link
Member

Extracts the shared tsconfiguration into a tsconfig-base.json file and has all tsconfig files extend from it. tsconfig-base.json also serves as a compilation entry point, so yarn tsc --build tsconfig-base.json compiles all the sub-(tsc-)projects.

Final PR to #12870, #12880, #12888, #12909, #12914, #12940, #12940, #12946, #12951, #12952, #12953, #12999, and #13069 sequence :)

#12860 listed some starting goals, but to summarize the point of all this:

  • made single declaration points for global LH.* types so they'd stop stomping on each other
  • clustered code so we'd stop duplicating compilation of files (like report renderer code compiled 4x since referenced from core test files, viewer, treemap, and flow-report) and simplify node vs DOM types
  • extract an LHR type independent of implementation code, hopefully for use somewhere
  • we can now emit d.tsfiles for our whole code base, in theory allowing easier import elsewhere (but I'm not sure we care about that anymore). Not suitable for publishing, though.
  • reduce type-check time with incremental compilation!

(all numbers from my laptop, means over five runs)
Before all this (ebb0b00):

> yarn type-check
Done in 28.11s 

This was every time you run yarn type-check.

After this commit with cleared compilation cache (rm -r .tmp/tsbuildinfo):

> yarn type-check
Done in 24.98s

Not doing multiple compiles of some files saves a bunch of time, but emitting d.ts and tsbuildinfo files for most of our code steals back a good bunch of it. flow-report/ was also added in the meantime. Small win.

But! If you've made no js/type changes and are just idly double checking everything is still good:

> yarn type-check
Done in 0.43s

Incremental also works within projects. e.g. a change to gather-runner.js only invalidates part of the core compilation and none of the report:

> yarn type-check
Done in 5.62s

And if you're working just in flow-report today, adding a new function to util.ts:

> yarn type-check
Done in 2.08s

Not going to win any esbuild awards yet, but much better :)

@brendankenny brendankenny requested a review from a team as a code owner September 15, 2021 22:51
@brendankenny brendankenny requested review from patrickhulce and removed request for a team September 15, 2021 22:51
@google-cla google-cla bot added the cla: yes label Sep 15, 2021
package.json Outdated Show resolved Hide resolved
tsconfig-base.json Outdated Show resolved Hide resolved
Comment on lines 26 to 34
"references": [
{"path": "./"},
{"path": "./types/lhr/"},
{"path": "./report/"},
{"path": "./report/generator/"},
{"path": "./lighthouse-viewer/"},
{"path": "./lighthouse-treemap/"},
{"path": "./flow-report/"},
],
Copy link
Member Author

Choose a reason for hiding this comment

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

it's arguable this should be the tsconfig.json since this sets up compilation of all of lighthouse and today's tsconfig.json be tsconfig-core.json or whatever. That way you'd be able to just do tsc -b (which will just get you the core tsconfig.json today). But it also feels weird to have tsconfig.json not touching real code, so 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused with the purpose of tsconfig.json at this point though, it's just to be referenced by the base for core?

My rough mental model would be something like

tsconfig-base.json, extended by all others, none of these references
tsconfig-core.json checks just core
tsconfig-all.json checks everything, has these references

We don't need to do exactly that, but is that reflecting the three needs we have at the root directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

tsconfig-base.json, extended by all others, none of these references
tsconfig-core.json checks just core
tsconfig-all.json checks everything, has these references

So yes, thanks, I skipped my reasoning here. This is a very common split (more or less the recommended structure).

Those are exactly our needs, but I hated having three files for it in the root. references aren't inherited by extending configs, and if tsconfig-base.json doesn't have anything in includes/files, it also doesn't use the compilerOptions itself, so we can just combine tsconfig-base.json and tsconfig-all.json from your list and it functions the same exact way.

But if that's just more confusing than having an extra file, we can just have the extra file :)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, all good now @patrickhulce. Let me know what you think about

But if that's just more confusing than having an extra file, we can just have the extra file

Copy link
Collaborator

Choose a reason for hiding this comment

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

if tsconfig-base.json doesn't have anything in includes/files, it also doesn't use the compilerOptions itself

I followed everything else but this. Why does it need to? It's only purpose from my perspective is to be the thing from which others inherit :) My expectation is that such a base file does not check anything in particular, just sets up the options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah crossed paths in replies :)

But if that's just more confusing than having an extra file, we can just have the extra file

Yeah I guess that's the side I'm on, 1 -> 2 feels like the rough part, 2 -> 3 is more like 🤷 and a win for clarity IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I guess that's the side I'm on, 1 -> 2 feels like the rough part, 2 -> 3 is more like 🤷 and a win for clarity IMO

sounds good. I like that it's more straightforward and we'll just have to expand our tsconfig zoo to make room :)

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