Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isolatedModules - potential issue with @types? #16351

Closed
johnnyreilly opened this issue Jun 8, 2017 · 8 comments
Closed

isolatedModules - potential issue with @types? #16351

johnnyreilly opened this issue Jun 8, 2017 · 8 comments
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@johnnyreilly
Copy link

johnnyreilly commented Jun 8, 2017

Hello!

I'm one of the maintainers of ts-loader, a TypeScript loader for webpack. We've recently been doing some work to improve performance by switching ts-loader to transpileOnly mode and using the fork-ts-checker-webpack-plugin to perform type checking in a separate process.

As a further optimisation we've been thinking about making use of the isolatedModules flag to make ts-loader do even less work. However when testing this out @odensc has stumbled upon this error:

ERROR at /home/oden/Dev/JS/fttv/node_modules/@types/babel-core/index.d.ts(16,19):
TS1205: Cannot re-export a type when the '--isolatedModules' flag is provided.

I wanted to raise this to check if there some kind of issue using isolatedModules in conjunction with @types?

Also, I wanted to request guidance on use of isolatedModules - there isn't much documentation on the flag that I've been able to find. Will setting the flag to true affect the experience of users in vscode / visual studio / alm-tools etc? Will less errors etc be reported if this flag is set?

I ask as, historically, ts-loader has used the tsconfig.json as is and has done no "magic" with it. However, if by setting isolatedModules to true then we degrade the IDE experience then I can see a reason for deviating from this. i.e. Having ts-loader setting isolatedModules to true internally if a certain ts-loader config option is set. But is this necessary? Does this flag have any bearing on the IDE experience? I simply don't know at present I'm afraid.

cc @jbrantly @piotr-oles @odensc @basarat

Possible related issue: #2812

@basarat
Copy link
Contributor

basarat commented Jun 8, 2017

Also, I wanted to request guidance on use of isolatedModules - there isn't much documentation on the flag that I've been able to find. Will setting the flag to true affect the experience of users in vscode / visual studio / alm-tools etc? Will less errors etc be reported if this flag is set?

isolatedModules helps prevent writing code that might break if a file is compiled individually vs. part of a compilation context.

Essentially errors pointed out by isolatedModule would block a reliable emit on a file by file basis i.e. using ts.transpile(individualFile).

So by isolatedModule you get more errors. e.g. global namespace usages will become errors.

Example:
file a

namespace t{
   export const foo = 123;
}

file b

namespace t{
   log(foo);  // ERROR in isolatedModules
}

Because we need to emit log(t.foo) which is something we can only know if file a is a part of the compilation context.

PS: Always a please to hear from you @johnnyreilly ❤️ 🌹

TS1205: Cannot re-export a type when the '--isolatedModules' flag is provided.

My dream (might not be valid) would be that isolatedModules has no effect on .d.ts files as they are not re-emitted. But of course I can be wrong ❤️

@mhegazy mhegazy added the Bug A bug in TypeScript label Jun 8, 2017
@mhegazy mhegazy assigned ghost Jun 8, 2017
@mhegazy mhegazy added this to the TypeScript 2.4 milestone Jun 8, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Jun 8, 2017

@Andy-MS we should not check this if we are in ambient context, regardless of the compiler options.

@johnnyreilly
Copy link
Author

johnnyreilly commented Jun 8, 2017

Thanks for advising @basarat - as helpful as ever! 💓

So it seems like I have isolatedModules backwards then! It's actually not going to improve performance in ts-loader at all!

Following your explanation I'm now curious; do you know of a good use case for isolatedModules? Short of writing a repl I can't think of one (and I'm totally sure it earns its keep there). But doubtless this is solving a problem?

@basarat
Copy link
Contributor

basarat commented Jun 8, 2017

t's actually not going to improve performance in ts-loader at all!

True.

Following your explanation I'm now curious; do you know of a good use case for isolatedModules?

If you know you are going to compile your code using ts.transpile you can set isolatedModules to true to warn you about any TS that might generate potentially invalid JS 🌹

@johnnyreilly
Copy link
Author

Cool - thanks 🌷

BTW ts-loader uses ts.transpileModule - is that any different to ts.transpile?

@basarat
Copy link
Contributor

basarat commented Jun 8, 2017

ts-loader uses ts.transpileModule - is that any different to ts.transpile?

Its the same. transpile is just a convenient wrapper around transpileModule and simply returns the outputText https://github.com/Microsoft/TypeScript/blob/441d76206578db4069c030b173ae660137532764/src/services/transpile.ts#L118 for when you don't care about diagnostics and sourcemaps 🌹

@johnnyreilly
Copy link
Author

Sweet - you are a fount of knowledge!

@ghost ghost closed this as completed in #16399 Jun 9, 2017
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jun 9, 2017
@ghost
Copy link

ghost commented Jun 9, 2017

If code is written to comply with --isolatedModules, it should be safe to transpile it without type information. (For example, you could use babel.) So this could have effects on performance. You could even transpile eagerly and type check using a separate process.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

3 participants