gulpjs / interpret

A dictionary of file extensions and associated module loaders.
MIT License
259 stars 47 forks source link

Disable ts-node typechecking #51

Closed weswigham closed 6 years ago

weswigham commented 6 years ago

When, eg, gulp uses interpret to load a Gulpfile.ts, ts-node will search the containing directory for all .ts files to run a typechecking pass on all of them (as of version 6) (instead of a module compilation with a root of the required file) - this is hugely unnecessary, and adversely impacts gulp startup time when using a .ts file (to the point where, in TS internally, we crash node because we have hundreds of MB of .ts in our test subrepos). By simply disabling the typecheck pass (and only using the fast-transpile behavior), startup returns to pre-ts-node-6 performance.

cc @blakeembrey for opinons, ref https://github.com/Microsoft/TypeScript/issues/23479

blakeembrey commented 6 years ago

SGTM. This has been a problem for ts-node also since users of the CLI expect type checking (e.g. TypeORM, etc) but gulp users want something quicker. I'd also prompt ts-node/register/transpile-only instead of using the callback style.

phated commented 6 years ago

Is there no way to keep the typechecking on but still avoid processing all files? I'm nervous to bring this in as a non-breaking change as someone might be relying on the type checking. And then I wouldn't really want to bump gulp-cli a major just for this.

blakeembrey commented 6 years ago

@weswigham Is there a way to use the TypeScript compiler without type checking everything? I used to filter by .d.ts files on startup, but that created another issue where people were writing definitions in .ts only and it wasn't being picked up. Maybe it's time to revert that change and make a prominent note on the README about the drawbacks?

weswigham commented 6 years ago

@blakeembrey imho, in most cases, just using the first require'd ts file as the only root file for the compilation usually works OK - then files you don't actually use don't get checked and built.

blakeembrey commented 6 years ago

@weswigham That didn't work unfortunately, because some people expect it to work like tsc - e.g. automatically pick up type definitions they've written. The old behaviour to work around this was to filter the list of found files to only .d.ts, but a few people had an issue with that too. I, however, don't have an issue with that since it's easily documented. That was what I wanted to get your thoughts on, unless it sounds like you think it's better to just use the input file and therefore skip any definitions the user may have tried to include to get compilation working?

weswigham commented 6 years ago

@blakeembrey that can be worked around with explicit triple-slash-refs in your entry point, so it's not an insurmountable issue (I'd take questions of how to specify command line arguments more seriously, because I see no direct in-source solution for that - maybe custom in-source pragmas?). And for this usecase specifically, working like tsc is usually undesirable - in smaller projects I'd sometimes have a tsconfig for my source in the same directory as my gulpfile, but that config most definitely would not apply to a gulpfile.ts in that folder (as the build orchestrator is not part of the build it orchestrates!).

And if you wanna claim it maps to a tsc call, you could just claim it maps to tsc entrypoint.ts (which spiders out from just that file) rather than tsc (which does discovery of all TS in the folder). In any case, that's the behavior that would suit gulp best, I think.

blakeembrey commented 6 years ago

@weswigham Not applying the tsconfig.json would be an issue, because we definitely can't default to always using es5 or ignoring your types. There's many varied consumers using the endpoint, including node -r ts-node/register (which is the same way Gulp, Webpack, etc. use it). However, I gave it a quick test and since the introduction of types and typeRoots it's less relevant to use files to reference .d.ts files. I'll make a new release with this change and document the behaviour.

blakeembrey commented 6 years ago

@weswigham New major released that skips files by default. This might be able to be closed now.

phated commented 6 years ago

@blakeembrey you rock!! I'll leave this open for a few days incase anyone stumbles over here. I was planning to do a once over on everything here next week.

phated commented 6 years ago

@blakeembrey I've read through the fallout to this change and I just wanted to say thank you for handling everything you've had to deal with.

I'm going to close this issue because it seems like the people running into problems (because they don't understand semver) are opening issues on ts-node and not here. And those issues show them how to set the environment variable for the old behavior.