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

support custom transformers for ts 2.3 #535

Merged
merged 4 commits into from
Jun 21, 2017
Merged

support custom transformers for ts 2.3 #535

merged 4 commits into from
Jun 21, 2017

Conversation

longlho
Copy link
Contributor

@longlho longlho commented May 3, 2017

TS 2.3.1 support custom transformer so I think ts-loader should support that too

@johnnyreilly
Copy link
Member

Sorry for the delay in responding - I've been pretty occupied with other things. Would you mind giving a few more details about this please? I'm guessing that your change is related to: microsoft/TypeScript#14419 ?

Having had a quick read I'm not totally clear as to the purpose of custom transformers - could you elaborate a little?

Also, it looks like you might need to resupply the PR as the changes for happypack compatibility collide with your changes. Finally, please could also provide a test? Thanks!

@johnnyreilly
Copy link
Member

Ping @longlho 😄

@longlho
Copy link
Contributor Author

longlho commented Jun 6, 2017

@johnnyreilly sry this slipped thru my attention. I believe the purpose of custom transformers is like babel-plugin: You have more control over what's being transformed as TSC walks thru the AST. 1 example is loading external asset (https://github.com/longlho/ts-transform-img).

This allows you to import * as img from 'foo.png' and output would be const img = 'base64_encoded_img' inline. Right now I have to pipe ts-loader into babel-loader and would love to get rid of babel-loader.

I'll rebase the PR :)

@Igorbek
Copy link

Igorbek commented Jun 19, 2017

Hey @johnnyreilly @longlho here's an example of a TS transformer for styled-components: https://github.com/Igorbek/typescript-plugin-styled-components. Since ts-loader doesn't support them, I have only section for integrating with awesome-typescript-loader.

@johnnyreilly
Copy link
Member

Thanks for sharing @Igorbek - I'm hoping @longlho is going come back and finish the PR soon. He said he would so I have high hopes!

@longlho
Copy link
Contributor Author

longlho commented Jun 19, 2017

yeah sorry I've been tied up. Will try to wrap this up this week

@johnnyreilly
Copy link
Member

Great - thanks @longlho! Could we use the same API as awesome-typescript-loader please?

getCustomTransformers?(): ts.CustomTransformers

We've a vague plan that atl and ts-loader may come together at some point. Either way, we want it to be easy for people to move from one loader to the other as required.

@longlho
Copy link
Contributor Author

longlho commented Jun 19, 2017

@johnnyreilly I fixed the PR. I'm trying to find a similar test to mimic but couldn't. Can you point me to something?

@johnnyreilly
Copy link
Member

Since there's no way for this to be tested at runtime (I think?) then it makes logical sense to create a comparison test rather than an execution test. I'd be tempted to use this one as a basis:

https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/basic

Remember you only need generate the 2.3 expected output as we only run comparison tests for the latest released build.

Do set me straight if you think this can be covered by an execution test but since it's about specifically using transformed output I'm guessing that doesn't make sense.

@johnnyreilly
Copy link
Member

Build failure BTW:

src/index.ts(189,9): error TS2345: Argument of type '{ compilerOptions: CompilerOptions; transformers: CustomTransformers; reportDiagnostics: true; fi...' is not assignable to parameter of type 'TranspileOptions'.
  Object literal may only specify known properties, and 'transformers' does not exist in type 'TranspileOptions'.
src/instances.ts(106,37): error TS2339: Property 'transformers' does not exist on type 'LoaderOptions'.

@Igorbek
Copy link

Igorbek commented Jun 19, 2017

As a test input, can be passed a simple transformer. I can write one if you want.

@longlho
Copy link
Contributor Author

longlho commented Jun 19, 2017

I think build might not be passing on 2.4.0-rc :-/ I tried changing the sub version and condition to support 2.4 but a bunch failed.

@johnnyreilly
Copy link
Member

Don't worry about 2.4 - that's a possible regression in TypeScript 2.4. see here: microsoft/TypeScript#16609 (comment)

@johnnyreilly
Copy link
Member

@Igorbek that'd be very helpful - thanks!

@johnnyreilly
Copy link
Member

(I haven't had time to fully understand custom transformers as yet)

@johnnyreilly
Copy link
Member

Yeah - looking at the build this all seems good. The failure seems to be TypeScript@next related and I don't think that's something that need concern us as I'm expecting that to be resolved in the compiler itself. All we need is a test and I think we're good to go! @Igorbek - if you get the chance it might be good to see if @longlho's fork works with https://github.com/Igorbek/typescript-plugin-styled-components as you would hope

@Igorbek
Copy link

Igorbek commented Jun 20, 2017

Here's a simple transformer that makes all string literals uppercase:

import * as ts from 'typescript';

const transformer: ts.TransformerFactory<ts.SourceFile> = (context) => {
    const visitor: ts.Visitor = (node) => {
        if (node.kind === ts.SyntaxKind.StringLiteral) {
            const text = (node as ts.StringLiteral).text;
            if (text !== text.toUpperCase()) {
                return ts.createLiteral(text.toUpperCase());
            }
        }

        return ts.visitEachChild(node, visitor, context);
    }

    return (node) => ts.visitNode(node, visitor);
};

export default transformer;

@johnnyreilly
Copy link
Member

Great! How would you expect that to be plugged into ts-loader's config?

@johnnyreilly
Copy link
Member

johnnyreilly commented Jun 20, 2017

Also; how would the output of that be expected to look? Am I right when I guess that this input:

const greeting = 'hi';

would become transformed into:

const greeting = 'HI';

?

@longlho
Copy link
Contributor Author

longlho commented Jun 20, 2017

yeah I think that's the output. The way custom transformer would be passed in is something like:

{
  loader: 'ts-loader',
  query: {
    getCustomTransformers: () => {
      before: [require('custom-transformer')],
      after: [require('some-other-transformers')]
    }                 
  }
}

@Igorbek
Copy link

Igorbek commented Jun 20, 2017

@johnnyreilly that's right, expected output is correct
@longlho yes, getCustomerTransformer should return { before?: Array<ts.TransformerFactory<ts.SorceFile>>; after?: Array<ts.TransformerFactory<ts.SorceFile>>; }

getCustomTransformers: () => ({ // note parens
      before: [transformer]
})

@johnnyreilly
Copy link
Member

Okay - I've had a first go at a comparison test based on @Igorbek's sample. Unfortunately it doesn't seem to work - I'm not too sure why though... BTW I built ts-loader with TypeScript@2.4.0 but I was using TypeScript 2.3.1 to generate the test data as that's what the test pack runs against. (So post build of ts-loader and pre-test I npm install typescript@2.3.1)

// 2. create a transformer;
// the factory additionally accepts an options object which described below
var styledComponentsTransformer = createStyledComponentsTransformer();

Copy link

Choose a reason for hiding this comment

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

The sample transformer exports the transformer itself, and not a factory function that typescript-plugin-styled-components does. So you don't need to call the function:

var uppercaseStringLiteralTransformer = require('./uppercaseStringLiteralTransformer').default;

loader: 'ts-loader',
options: {
getCustomTransformers: () => ({ // note parens
before: [styledComponentsTransformer]
Copy link

Choose a reason for hiding this comment

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

before: [uppercaseStringLiteralTransformer]

@johnnyreilly
Copy link
Member

Thanks @Igorbek - just made those changes. Looks promising!

Copy link

@Igorbek Igorbek left a comment

Choose a reason for hiding this comment

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

Looks very nice!

Asset Size Chunks Chunk Names
bundle.js 2.92 kB 0 [emitted] main
chunk {0} bundle.js (main) 193 bytes [entry] [rendered]
[0] ./.test/customTransformer/SUBMODULE/SUBMODULE.ts 78 bytes {0} [built]
Copy link

Choose a reason for hiding this comment

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

transformer uppercased the module import as well :) it was unintentional, but ok ))

Copy link
Member

Choose a reason for hiding this comment

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

In actual fact that is the reason the test is failing on Travis right now I think. Case insensitive Windows Vs case sensitive Linux 😄 I'll change the test so it is 1 file only (no imports) tonight and then we should have a reliable test that works cross platform. Then I think we're ready to merge!

@johnnyreilly
Copy link
Member

Okay - so we've now got a cross platform test! However, it looks like customTransfrmers aren't applied in transpileOnly mode. Is that intentional?

@longlho
Copy link
Contributor Author

longlho commented Jun 21, 2017

hmm nope those 2 configs should be independent

@johnnyreilly
Copy link
Member

If you look at TranspileOptions it doesn't appear to a valid option:

    interface TranspileOptions {
        compilerOptions?: CompilerOptions;
        fileName?: string;
        reportDiagnostics?: boolean;
        moduleName?: string;
        renamedDependencies?: MapLike<string>;
    }

@johnnyreilly
Copy link
Member

So maybe this isn't supported for transpileOnly?

@longlho
Copy link
Contributor Author

longlho commented Jun 21, 2017

@Igorbek
Copy link

Igorbek commented Jun 21, 2017

yes, 2.4rc supports it.

@johnnyreilly
Copy link
Member

Ok - so the test was generated with 2.3 so if it was added with 2.4 that makes sense. If you use it with 2.4 it'll just work?

@johnnyreilly
Copy link
Member

I'm happy - let's merge!

@johnnyreilly johnnyreilly merged commit 6594328 into TypeStrong:master Jun 21, 2017
@johnnyreilly
Copy link
Member

I'm cutting ts-loader 2.2 right now - should be released within the hour. Thanks for the contribution!

@TomMarius
Copy link

TomMarius commented Jul 6, 2017

Hey guys, how do I get to the type checker (ts.TypeChecker)? The method getTypeChecker is available only in the ts.Program interface and I don't see it being passed down from the loader nor can I find any examples that would use the type checker. I'm probably just missing something, but I've been scratching my head for a few hours now.

@johnnyreilly
Copy link
Member

Do you mean fork-ts-checker-webpack-plugin? It's a separate npm package: https://github.com/Realytics/fork-ts-checker-webpack-plugin

@johnnyreilly
Copy link
Member

@TomMarius
Copy link

Nonono, I meant how to get to the TypeScript's internal instance of TypeChecker that I use in my custom transformers. Thank you for your reply though.

@TomMarius
Copy link

TomMarius commented Jul 6, 2017

See this custom transformer for example: https://github.com/kimamula/ts-transformer-enumerate/blob/master/transformer.ts

He uses argument program of function visitNode to pass down the compiler's instance of the Program interface. He then uses the getTypeChecker method and then uses the TypeChecker instance to resolve types in the transformer. I need to get either the TypeChecker itself or an instance of the Program interface down from ts-loader.

@johnnyreilly
Copy link
Member

Ah I see. To be honest I've not really done anything with transformers and so can't help. Maybe @longlho or @Igorbek can...

@longlho
Copy link
Contributor Author

longlho commented Jul 6, 2017

@TomMarius you can't get access to the program currently since it's out of scope. You will need to write a custom compiler to hoist that or PR'ed to TS have that passed down as part of the TransformationContext. I guess ts-loader can pass that down as a param, althou I'm not sure how future-proof that will be.

@TomMarius
Copy link

@longlho Okay, I'll probably create a PR in the following days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants