Add declaration transform stage that combines repeated import type nodes into single import declarations

[weswigham]

And a prettyDeclaration flag which defaults to true, but may be toggled to false to disable this new behavior (and thus any performance cost can be opted out of). There's a some cost to doing this work (since we need to build a list of imports we need to make and import specifiers that are safe to use), but it does result in a much nicer output in cases where many import types were used before, and the output can theoretically be faster to read back in sometimes, so the choice is a bit of a tradeoff. Still - we default to this new behavior any time declaration is explicitly set to true - incremental without declaration doesn't turn on this new behavior, since incremental declaration files don't need to be pretty.

Fixes #44044

[sheetalkamat]

Dont we need to handle composite which means declaration = true?

Suggested change

	return compilerOptions.prettyDeclaration !== undefined ? !!compilerOptions.prettyDeclaration : !!compilerOptions.declaration;
	return compilerOptions.prettyDeclaration !== undefined ? !!compilerOptions.prettyDeclaration : getEmitDeclarations(compilerOptions);

[weswigham]

Nope! composite declarations are only used for checking signatures, and not for human-readable output purposes; so I've explicitly not enabled the pretty pass when only that is set.

[sheetalkamat]

when someone sets composite it also means emit declaration.. It isn't just for signature purpose

[Mifrill]

That's a good point! Perhaps, we can use getEmitDeclarations directly instead of checking both prettyDeclaration and declaration separately:

Suggested change

	return compilerOptions.prettyDeclaration !== undefined ? !!compilerOptions.prettyDeclaration : !!compilerOptions.declaration;
	return getEmitDeclarations(compilerOptions);

[sheetalkamat]

This isnt checking if this transform is for incremental signature purposes but i think its a good idea to do it this way.. We try to update signatures to d.ts emit hash during emit so not having discrepancies between d.ts emit for incremental signature purpose and emit is better..

[sheetalkamat]

can you also add one of this as tests that baseline edits as well:

• incremental with declaration
• incremental with composite
• incremental without both the flags

[amcasey]

The goal of #44044 was to reduce emit time, but the description makes it sound like this might increase it?

[weswigham]

"might"? Absolutely does more like. You could maybe eke out better but still worse perf by contorting some checks into earlier passes to save on walks, but it's an exercise in futility, imo. It's why I say this is purely a style thing and can be opt-out. At least on the emit side. It's potentially a performance gain when we read the declaration file back in (either for library consumers or incremental building) because there's fewer nodes in the generated declaration file. I never thought this'd be good for emit perf - it requires nonlocal scope analysis (and specifier generation itself is already cached).

[weswigham]

As the related issue says:

Cumulatively, these can significantly enlarge a declaration file.

And indeed, this makes the output file smaller. But that does cost time. It's very much a case-by-case basis weather that time is worth it, hence the available toggle.

[amcasey]

As the related issue says:

Cumulatively, these can significantly enlarge a declaration file.

And indeed, this makes the output file smaller. But that does cost time. It's very much a case-by-case basis weather that time is worth it, hence the available toggle.

I have no interest in this as a style change, but it's possible that others might. #44044 was only intended to refer to the performance gain I hoped we might achieve.

[robpalme]

IMO this is a worthwhile change because the deduplication:

• makes it easier to for a human to read the DTS
• makes it easier to review deltas over time to the DTS
• reduces the file size published to definitelytyped
• reduces the consumption cost of DTS

A further extension might be to group all synthesised imports at the top of the file with an explanatory comment. This makes it simple to see what kind of dependencies the DTS files really have.

[Mifrill]

Suggested change

	// TODO: Since imports are hoisted (albeit with TDZ), we technically need to modify every import declaration _before_ recuring
	// on import type nodes to modify those, since technically the import an import type node relies on can occur _after_ it in
	// a file. This is somewhat unfortunate since it makes this require 3 tree walks - one to gather the imports and import type nodes we care about,
	// one (albeit likely shallow one) to modify the existing import declarations, and one to update all the import type nodes themselves.
	// TODO: Since imports are hoisted, albeit with a Temporal Dead Zone (TDZ), we technically need to modify every import declaration _before_ recurring on import type nodes.
	// This is because the import an import type node relies on can occur _after_ it in a file.
	// Unfortunately, this makes the process require three tree walks: one to gather the imports and import type nodes we care about, one (likely shallow) to modify the existing import declarations, and one to update all the import type nodes themselves.

[sandersn]

@weswigham do you want to pursue this any further?

[sandersn]

It's been almost 3 years since the last change to this PR. I'm going to close it for now and we can re-open it if it's worth working on before the Strada code is decommissioned.