separate out "ts" from "tsx"

[evanw]

Hello! First of all, thanks for creating and maintaining this loader.

I noticed that this project recommends always using the tsx loader even for .ts files, and then tries to automatically fall back to using the ts loader in cases where there is a syntax error. I'm reaching out to let you know that this is a performance and correctness issue, and to propose one way of fixing it.

It's not valid to compile .ts files in tsx mode. The tsx syntax is not a superset of the ts syntax. Instead they are two separate but related syntaxes. Here is an example:

export default () => <a>1</a>/g

This file has two valid compilations, one in each syntax:

// When compiled with "ts"
export default () => 1 < /a>/g

// When compiled with "tsx"
export default () => React.createElement("a", null, "1") / g

With the approach of always using the tsx loader, you will incorrectly always end up with the second compilation. I gave this example because it's a case where both loaders succeed. But there are other cases where the approach of using the tsx loader first and then trying the ts loader next will fail to handle valid TypeScript code. This example should compile fine with ts but doesn't compile with tsx, and this loader doesn't fall back to the ts loader afterward:

export let a = <number>foo;
export let b = '</number>';

I don't think you should fix this by extending the set of cases in which the ts loader is tried after the tsx loader fails. Not only is doing this fallback not correct in all cases (e.g. the regular expression case above), but it's also a performance issue because it causes esbuild to run twice for TypeScript files with type casts in them. Given that people are using this loader primarily for the performance benefit, I think it would be a good idea to make sure this loader is only run once.

The simplest fix is just to update the documentation to recommend configuring a separate loader for .ts files and .tsx files and to remove the fallback from tsx to ts. I suggested this fix because it requires minimal code changes. Note that this is a breaking change for existing users.

I just made this a PR instead of an issue so I could show you one way of fixing it. But feel free to close this PR if you would like to fix this a different way. If you want to keep the simplicity of being able to specify a single /\.tsx?$/ rule, you will need to change your loader to decide to use either the ts loader or the tsx loader depending on the file extension of the input file.

[privatenumber]

Thanks for this thorough explanation @evanw, and also for all your incredible work on esbuild.

The reason this fallback was added was because there seemed to be confusion around configuring esbuild-loader correctly (#53). Making the configuration API as close to ts-loader as possible seemed to be the most intuitive:

{
  test: /\.tsx?$/,
  loader: 'ts-loader'
}

Given your explanation though, I'm convinced this fallback logic should be removed. However, I'd like to avoid the breaking change (at least in API), and more importantly, keep the API as close to ts-loader's so that migration changes are easy & minimal.

It seems like ts-loader checks whether the target file has the tsx extension or not and uses that as a signal to determine whether to compile as TS or TSX. I'm thinking of changing the logic to that via patch. What do you think of this solution?

[evanw]

However, I'd like to avoid the breaking change (at least in API), and more importantly, keep the API as close to ts-loader's so that migration changes are easy & minimal.

It seems like ts-loader checks whether the target file has the tsx extension or not and uses that as a signal to determine whether to compile as TS or TSX. I'm thinking of changing the logic to that via patch. What do you think of this solution?

Sure, sounds good! I'll close this then since you're now aware of the issue and have decided on a different approach. Thanks for being thoughtful about fixing this.