Simplify build; de-hardcode typescript typings

[use-strict]

Some questions and things I might have gotten wrong:

• Why rely on the global typescript module for building ts-loader? Build dependency should be well fixed. Only runtime version varies.
• Do we need a build.js file if we just ran tsc with no arguments?
• The signature for readConfigFile seems wrong, even for older typescript versions.
• I would also add "noImplicitAny" as a compilation option.
• Is there a reason for still supporting TS < 1.6 for building ts-loader?

P.S. Sorry about the whitespaces, my editor trims trailing whitespaces by default and I didn't notice before creating the pull request. But it is something you should also use.

[jbrantly]

Related: #76

[use-strict]

My bad! Only looked in issues, forgot to check PRs.

tsconfig.json will work with 1.5.3, even with unrecognized options, but in my PR I'm relying on node module resolution, so it won't find typescript.d.ts in this case. Question remains. Is there any reason why you would want ts 1.5.3 for build step?

[jbrantly]

One quick issue. If you look at the automated tests you'll see that the "typescript@next" test passed but was actually run against TS 1.6.2 and not TS 1.7.

The reason that the TS definitions are hardcoded is that they represent the run-time version which can vary between 1.5.3 and 1.7. For instance, readConfigFile does have that signature in 1.7.

Is there any reason why you would want ts 1.5.3 for build step

For development of features/bug fixes targeting 1.5.3, it just made it easier (see last few comments in #76). But you're right and I had planned to make another commit to #76 which dropped it.

[use-strict]

I see. Well the hardcoded typescript.d.ts did not include this signature either and compiler was an implicit any so I assumed it was a mistake.

Can you clarify what is exactly in typings/typescript.d.ts? Is it custom-made aggregation between multiple definitions or was it copied as is from the microsoft repo?

[use-strict]

Yes, in .travis.yml there is an npm install after npm install $TYPESCRIPT. Should be the other way around?

[jbrantly]

was it copied as is from the microsoft repo

It's copied as-is, just a little old perhaps. Since I only use it for a few structures that haven't really changed much it hasn't been critical to keep it updated.

compiler is any because of the runtime version support (transpile vs transpileModule, resolveModuleName, whatever else happens in the future). I could certainly see a different way of approaching this by having a custom-made aggregation for typescript.d.ts.

Should be the other way around?

Yup!

[use-strict]

Supporting multiple typescript versions at runtime would surely complicate the code, if you were to have proper typing. Typing compiler as any is not correct imho, even if you don't know what version you are using. If we were to be very strict here, we would be typing it as a GenericCompiler which exposed only a version property at minimum and then use a union between known compiler signatures or something along those lines. You would have to explicitly typecast based on intended version which would make your code very hard to manage. Also, it will magically work with typescript@next on the wishful thinking that its signature will look like typescript@latest. Otherwise you would have to add that signature explicitly.

I would advise dropping support for older versions and only maintaining latest and next at runtime. If someone wants older versions, there is always npm install [email protected].

[jbrantly]

I would advise dropping support for older versions and only maintaining latest and next at runtime.

Keep in mind that TS 1.6 stable released less than a month ago 😁 1.5 is definitely getting dropped in ts-loader v0.6, just haven't gotten there yet is all.

Typing compiler as any is not correct imho

I don't disagree, but both approaches have tradeoffs. I would certainly be open to exploring using the node resolution to grab TS's build-time typings and then using module merging to add any typings from typescript@next.

Re: using a version string, I want to avoid that approach because I want to support unofficial compilers like ntypescript. Instead I prefer to do feature detection like today. But I don't think that's incompatible with the goals here.

[jbrantly]

Why don't we do this... let me finish up #76 and get that merged. Then let's turn this PR into fixing up the definitions?

[use-strict]

OK, just saw your comment now. I did exactly what you didn't want, which is to do version detection. Tests pass though (1.5.3 should be removed from the tests).

v0.6 is just a label. I agree it should not be in 0.5.x for semver reasons, but is there a problem going from v0.5.6 to v0.6 with this PR? :)

Is there any way to detect which version of readConfigFile we should be using? I know at runtime it will work even with extra arguments being passed, but we are relying on a side-effect and this won't work for signatures that are completely diffferent. I also see no way to detect this at runtime, so we need a way to detect this at compile-type, even for custom typescript builds.

Idea: We could pick the correct signatures, based on a config option or some extra metadata taken from the custom typescript package. That is assuming it still matches something we know.

[jbrantly]

but is there a problem going from v0.5.6 to v0.6 with this PR

Nope. Sorry, I wasn't trying to say that we couldn't drop 1.5, just why we hadn't yet. I saw no reason to cut a release for the sole purpose of dropping 1.5. I just wanted to wait until there was a reason (like this PR). Less work.

this won't work for signatures that are completely diffferent

While that's true, I'm taking a "don't cross that bridge until I have to" approach. So far every situation I've ran into (and there have been a few) I've been able to resolve using feature detection. If there is some change where we can't use feature detection then yea we'll have to base it on something else.

[jbrantly]

@use-strict Sorry it took me so long to get #76 in (work has been crazy this week), but I just merged it. If you want to continue with this PR you can do so now. I want to remove support for 1.5 and 1.6.2 1.6 beta so if you run into any issues with those feel free to remove.

[jbrantly]

Whoops! I meant 1.6 beta, not 1.6.2.

[use-strict]

I think that changing the order of the npm installs doesn't run the build script again with the new tsc. But I think it worked before the merge which is odd. And the compilation fails now with tsc 1.8 dev. Maybe they changed something again from 1.7, I will check.

[use-strict]

Looks like parseConfigFile is gone with the latest dev tsc. I don't know how to fix this atm.

index.ts(245,38): error TS2339: Property 'parseConfigFile' does not exist on type 'typeof ts'.

Edit: Looks like they renamed it to parseJsonConfigFileContent.

[use-strict]

OK, all done. Tests for 1.5.3 should be removed.

[jbrantly]

This needs to be changed back to program. The language service doesn't come into play here.

[use-strict]

getCompilerOptionsDiagnostics doesn't exist on program. Only on the language service object. It won't even compile like that. See typescript.d.ts. Am I missing something here?

EDIT: I'm guessing it could have been like that in older versions. That's maybe why you check if getOptionsDiagnostics exists? program was initially typed as any, but now I don't think that makes sense anymore since you will get a compile error if getOptionsDiagnostics doesn't exist in the definition. Is the else branch needed at all?

[jbrantly]

Ah, you're right. I think getCompilerOptionsDiagnostics must have been there at some point in the nightly or something. Looks like that branch can just be removed and this can just be diagnostics = program.getOptionsDiagnostics();

[jbrantly]

Looks like parseConfigFile is gone with the latest dev tsc.

Yea, sorry. I had already opened #83 about it. I should have mentioned it.

[jbrantly]

The last remaining piece before merging is the version checking. I really want to shy away from that for various reasons. Perhaps you could make a TSAllCompiler or something which contains overloads from all supported versions and then simply do property detection (much like the code does today, but with typing).

[use-strict]

I don't see how this would be possible. We just cannot safely detect the signature of a function at runtime.

I don't want to rely on runtime hacks, which are not guaranteed to work all the time and may fail at any moment. Maybe we could get away now with passing an extra parameter to readConfigFile and check the existence of the parseConfigFile, but that's not maintainable in the long run. Just imagine the next version changing the type of the second parameter. What then? This also makes code harder to understand and to cleanup later when older API versions will be dropped.

Maybe you could let me in on those "various reasons" so I can better understand them. IMHO, doing feature detection on an API makes no sense. If the API has multiple versions, we select the version we want explicitly.

[jbrantly]

I don't see how this would be possible. We just cannot safely detect the signature of a function at runtime.

Practically, it's worked for me for the lifetime of this project, starting with TS 1.4, picking up ntypescript when it came out and then of course the officially nightly. I've never had to resort to a version check. TS APIs tend to evolve, either being added to or renamed but rarely outright changed.

Just imagine the next version changing the type of the second parameter. What then?

Then maybe we need a version check at that time.

This also makes code harder to understand and to cleanup later when older API versions will be dropped.

Not if good comments are used, which I haven't been consistent with but should be.

Maybe you could let me in on those "various reasons" so I can better understand them. IMHO, doing feature detection on an API makes no sense.

Some of the most common code on the web for a long time was:

if (element.addEventListener) { element.addEventListener(...) } 
else { element.attachEvent(...) }

Reasons:

• I want to continue to support non-official TypeScript forks like ntypescript. For awhile jsx-typescript was a pretty awesome fork. Using version sniffing would effectively eliminate the compiler option.
• While it obviously happens that the nightly can outpace the loader, I don't want the loader to outpace the nightly. Meaning, ideally I don't want someone with an early version of the 1.7 nightly upgrading ts-loader and finding things now break. You would have to combat this by using very specific versions of nightly in your version check, not the broad 1.7.0-0 check.
• Reasons given above like "it's worked well so far"

[use-strict]

Forks or custom builds of typescript indeed complicate things. I didn't quite consider this scenario. So the project is typed with a known compiler, while allowing anything that looks like a compiler at runtime.

You're right in this case that we can't do version sniffing. But we can also only assume what that compiler API looks like. We'd have to impose some restriction on it, like "should be compatible with official 1.6.2+ API". I can't think of a clean method to fix this right now, will chew on this some more.

I've never had to resort to a version check. TS APIs tend to evolve, either being added to or renamed but rarely outright changed.

I will then go on this assumption and hope we never have to cross that bridge. I have to say that I got bitten quite a few times on similar assumptions though. Hope it won't be the case here.

if (element.addEventListener) { element.addEventListener(...) }
else { element.attachEvent(...) }

Your analogy with browser DOM API is more appropriate in the context of an unknown compiler. I was thinking more in the terms of a REST-like API where the behavior could change from version to version, even though the API calls may be the same.

[jbrantly]

We'd have to impose some restriction on it, like "should be compatible with official 1.6.2+ API"

For what it's worth there is already some functionality similar to this. See the "motd" related code and the message "This version may or may not be compatible with ts-loader". I wouldn't mind changing/appending that with something like what you mentioned.

[use-strict]

See last commit.

[jbrantly]

Sweet! Thanks! Sorry it took so long, thanks for being patient with me.

[use-strict]

Same goes. Thanks!