Use compiler enum than magic number

[HerringtonDarkholme]

fix #86

[johnnyreilly]

Cool! Is this still an issue though?: microsoft/TypeScript#4690

See the point made by @jbrantly here

[HerringtonDarkholme]

Don't worry, I only access compiler in this case. So no redundant require will be generated.
It will even be resistant to enum value change for version upgrade but that's quite once in a blue moon for typescript team

[johnnyreilly]

LGTM 👍

[HerringtonDarkholme]

actually I found typescript has been required in servicesHost, in getDirectories. I guess we want to avoid requiring typescript because we want to support custom compiler and does not require user to install both tsc and custom copmiler.

[johnnyreilly]

That's a good point. I'm wondering if I've changed something by mistake or if @jbrantly previously decided to use this approach. @jbrantly would you be able to provide any insight here? Is it ok to be requiring typescript as we are or is that something we ought to avoid?

It looks like using import typescript = require('typescript'); predates the refactor:

ts-loader/index.ts

Line 6 in 9a2e408

import typescript = require('typescript');

[johnnyreilly]

BTW @HerringtonDarkholme ts-loader 1.1.0 (with vue support) has 🚢 😄

[HerringtonDarkholme]

@johnnyreilly can this be merged?

[johnnyreilly]

Well @jbrantly hasn't come back with a reason why not. That being the case I figure we should merge this; since we seem to be import typescript = require('typescript'); already Feel free to merge!

[jbrantly]

Apologies, I missed the first mention. The original idea was to not require('typescript') in the output generated by TypeScript so as to be able to support custom compilers, that's correct. However, the TypeScript definitions were still used, which didn't generate a require as long as only metadata was accessed. At the time, accessing a constant enum also generated a require. It seems like that's still the case, so this could still be an issue.

[jbrantly]

Regarding the current use of typescript.sys in servicesHost, I think it should actually be compiler.sys. It doesn't look like typescript.sys was accessed in the original code.

[johnnyreilly]

Regarding the current use of typescript.sys in servicesHost, I think it should actually be compiler.sys. It doesn't look like typescript.sys was accessed in the original code.

You're right - I'll take a look at this.

At the time, accessing a constant enum also generated a require. It seems like that's still the case, so this could still be an issue.

In that case let's not merge for now. When that issue is resolved let's merge then

[HerringtonDarkholme]

At the time, accessing a constant enum also generated a require. It seems like that's still the case, so this could still be an issue.

I don't think this is an issue because we can always use compiler's enum so no require will be generated.

[johnnyreilly]

I see the red 😭

It only occurs on the 2.1.1 build. Without having looked into it I'm going to speculate that is as expected:

• the comparison tests only run against 2.0 and 2.1
• this changes the compiler usage and so may well be as expected

To avoid confusion I suggest we do the following:

1. initially fix the TypeScript -> compiler issue. Address any changes in the comparison test pack that may arise.
2. reapply the initial enum fix this PR raised. Compare the emitted JavaScript for compilerSetup.ts and servicesHost.ts before and after the fix. Share our findings and discuss. That way we're making the decision on the back of actual code and it becomes a simple case of "let's do this now" or "let's wait" for concrete reasons.

I can look into 1. but probably not until this weekend as I've a very busy week ahead. I don't mind if others want to pick this up first - but if no-one does I will.

[johnnyreilly]

Oh and let's do 2 as distinct from 1 to avoid confusion

[johnnyreilly]

Okay I've done 1. Took less time than expected

[jbrantly]

I don't think this is an issue because we can always use compiler's enum so no require will be generated.

That's a good point and something I didn't realize when first looking at this. I believe you're probably right.

[HerringtonDarkholme]

I gave up

https://github.com/Microsoft/TypeScript/blob/master/src/compiler/types.ts#L3197-L3205

[johnnyreilly]

I don't get the reference? Were there problems with ScriptTarget? Oh - this is because ES6 has become ES2015 and so on isn't it?

[johnnyreilly]

Good effort though! 👍

[HerringtonDarkholme]

this is because ES6 has become ES2015 and so on isn't it?

Exactly. Then to support both TS2.1 and below we need to add another layer of abstraction, and it is workaround anyway. That just does not worth. I would prefer magic number workaround.