Remove duplication from transformers

[danielbodart]

Fixes #28310, #24459, cevek/ttypescript#29

This pull requests removes duplication (chainBundle) from all the transformer functions and moves it to a single place (getTransformers).

This will then fix custom transformers so they work with single outFile compiler option (Bundle being passed) and gets rid of the need for chainBundle to be exposed.

This pull request is a refactoring to remove duplication and increase consistence by having a single way all transformers are used.

[msftclas]

All CLA requirements met.

[danielbodart]

Also replaced createBundle in chainBundle with version from transformTypeScript so it correctly handles InputFiles. Then removed from transformTypeScript so no duplication

[weswigham]

It's worth noting that declaration after-transformers still won't be auto-chained (they don't use getTransformers, ofc) - this also precludes the ability for any custom transformer to specifically handle bundles (potentially by unbundling or by adding prepends).

[weswigham]

(I think the other option proposed in the linked issue:

1. leave everything as it is and write in the documentation that the transformer developer should support Bundle nodes.

Is probably better - we should update the docs to say that a custom transformer should handle bundle nodes and then expose chainBundle for use)

[danielbodart]

@weswigham I'm not sure I agree. You are making the normal use case suffer for the exception. Every single custom transformer that I have come across in the wild (mainly from the https://github.com/cevek/ttypescript home page) would need to be updated rather than they just work as expected.

I would argue it would be better to have a separate interface/contract for bundle transformers (if anyone actually wanted to do that). I'd probably make the method take a Bundle and return a Bundle, rather than passing the complexity down with union types.

[danielbodart]

On the subject of declaration after-transformers, they already have a different type signature so it's probably too late to fix them but if it was my code I would simplify them and separate Bundle transformation as I mentioned previously.

[weswigham]

Except the declaration transformer meaningfully transforms bundles, so it actually cares about them (quite a lot). I'm simply stating that it's not unlikely that a set of other transforms may exist which also care about weather or not files are being bundled and would like to manipulate that bundle.

[rbuckton]

this also precludes the ability for any custom transformer to specifically handle bundles

This is a very good point. This could potentially be a breaking change for custom transforms.

Potential breaking change.

[danielbodart]

Hmmm I don't think that's correct, right now customer transformers (excluding decorators as they have a different contact) can't transform bundles at all. They are broken when bundles are used. If you changed the signature for them you would break every single one out there. Changing an argument type to a union type is not backwards compatible.

This change is backward compatible, fixes them so they all work with bundles correctly with zero change and gets rid of a bunch of duplication. This change does not effect declaration transformers.

I would argue creating bundle transformers should be a separate method on custom transformers but it's out of scope for this change.

[danielbodart]

@rbuckton Any more thoughts on this?

[weswigham]

Like, if we're currently passing bundles to transformers, the type declaration is what's wrong, IMO - it should reflect that we're passing bundles (since anyone who's noticed the mismatch is being forced to handle them right now anyway).

[rbuckton]

Ideally a custom transformer should be able to take a bundle, but I agree that adding Bundle to a union is problematic. Instead of the changes mentioned in this PR, we should consider wrapping the custom transformers to handle bundles, since our API today only allows custom before and after transformers for SourceFile:

    export interface CustomTransformers {
        /** Custom transformers to evaluate before built-in .js transformations. */
        before?: TransformerFactory<SourceFile>[];
        /** Custom transformers to evaluate after built-in .js transformations. */
        after?: TransformerFactory<SourceFile>[];
        /** Custom transformers to evaluate after built-in .d.ts transformations. */
        afterDeclarations?: TransformerFactory<Bundle | SourceFile>[];
    }

We would need to consider a different mechanism to indicate whether a custom transformer supports Bundle, perhaps by modifying the definition as follows:

export type CustomTransformerFactory = (context: TransformationContext) => CustomTransformer;

export interface CustomTransformer {
    transformSourceFile(node: SourceFile): SourceFile;
    transformBundle(node: Bundle): Bundle;
}

export interface CustomTransformers {
    /** Custom transformers to evaluate before built-in .js transformations. */
    before?: (TransformerFactory<SourceFile> | CustomTransformerFactory)[];
    /** Custom transformers to evaluate after built-in .js transformations. */
    after?: (TransformerFactory<SourceFile> | CustomTransformerFactory)[];
    /** Custom transformers to evaluate after built-in .d.ts transformations. */
    afterDeclarations?: (TransformerFactory<Bundle | SourceFile> | CustomTransformerFactory)[];
}

[rbuckton]

I've submitted #31301 as an alternative to this approach.

[rbuckton]

Closing this PR as #31301 has been merged