ds300 / react-native-typescript-transformer

Seamlessly use TypeScript with React Native
MIT License
657 stars 55 forks source link

Ability to configure the transformer by creating a custom transformer #65

Closed ajcrites closed 6 years ago

ajcrites commented 6 years ago

This pull request has a bit of a background. I'm trying to set up a consistent React Native build process for my organization and minimize friction. We're using TypeScript, and this tool has been great.

In spite of the fact that the community frequently uses multiple TypeScript configurations (often for testing and production), TypeScript doesn't support specifying a TypeScript config for tsserver: https://github.com/Microsoft/vscode/issues/12463 which prevents you from configuring quite a few tools that use tsserver.

After looking at React Native and Metro, I figured that the simplest way to be able to configure TypeScript for transforming would be here. We do have TSCONFIG_PATH available, but I want to minimize friction for developers so they only have to set additional configuration or take additional steps when absolutely necessary.

Due to the (imo) weird way that Metro works, you have to specify a path for the transformer. To this end, I've updated this module to export function to create the transformer. This will allow users of the module to set configuration by creating custom transformers (see the README update). It's awkward, but this seems to be the simplest way.

I tried my hardest to keep this fully backwards compatible.

ds300 commented 6 years ago

Thanks! I appreciate that env vars can be a little unwieldy.

It's not obvious, but you can also do this in your rn-cli.config.js:

+ process.env.TSCONFIG_PATH = require.resolve('./my-tsconfig-file.json')
 module.exports = {
   getTransformModulePath() {
     return require.resolve('react-native-typescript-transformer');
   },
   getSourceExts() {
     return ['ts', 'tsx'];
   }
 }

Does that solve your problem?

ajcrites commented 6 years ago

@ds300 I'll go with that if you don't like the PR, but I don't like to write directly to environment variables in this way in scripts. It may be perfectly fine to do so, but it just seems funky to me, and I think it's undocumented. The ability to set configuration will also allow us to set more options if they become available ... I just couldn't think of any (maybe TypeScript configuration options themselves would be fair game).

Ideally we could just do the configuration here, but since Metro requires a path rather than a transformer function this is the best way I found to set custom configuration that doesn't rely on environment variables ... super-globals if you wish.

ds300 commented 6 years ago

I think mutating process.env is quite safe to do. It's how dotenv works and that gets millions of downloads a week. If node changed this behaviour it would break a lot of projects.

As for other configuration options, that might make sense at some point in the future. I'll cross that bridge when I come to it :-) maybe I'll even dig up the code in this PR!

Thanks for going to the trouble of submitting a PR. It makes me super happy when people show an interest in contributing 🙌

ajcrites commented 6 years ago

Sure thing! I plan to use this transformer quite a bit, so I'll see if I can find some ways to contribute as needed.