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

TypeScript can see objects globally if they are not exported. #31822

Closed
sergey-shandar opened this issue Jun 8, 2019 · 11 comments
Closed

TypeScript can see objects globally if they are not exported. #31822

sergey-shandar opened this issue Jun 8, 2019 · 11 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@sergey-shandar
Copy link
Contributor

sergey-shandar commented Jun 8, 2019

TypeScript Version: 3.5.1

Search Terms:

Code

File ./index.ts

class MyPrivateClass {}

File ./test.ts

const x = new MyPrivateClass()

File ./tsconfig.json

{
  "compilerOptions": {
    "target": "es2015",
    "module": "commonjs",
    "strict": true,
  }
}

Command

tsc

Expected behavior:

TypeScript compiler should report an error in ./test.ts that MyPrivateClass is not visible.

Actual behavior:

TypeScript doesn't report any error and generates invalid ./test.js and ./index.js files. For example, node ./test.js are failing with this issue:

const x = new MyPrivateClass();
          ^

ReferenceError: MyPrivateClass is not defined
    at Object.<anonymous> (C:\github.com\ts-common\visibility-test\test.js:2:11)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:741:12)
    at startup (internal/bootstrap/node.js:285:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:739:3)

Playground Link: No playground link because we need two files but here's GitHub repository https://github.com/ts-common/visibility-test/tree/d3c03a1cd4641da1bd1356e751e2cea10ea91025

Related Issues: I couldn't find any.

Note:
TypeScript compiler will give a compilation error only if the class is exporter:

export class MyPrivateClass {}
@nattthebear
Copy link

https://www.typescriptlang.org/docs/handbook/modules.html

Conversely, a file without any top-level import or export declarations is treated as a script whose contents are available in the global scope (and therefore to modules as well).

@sergey-shandar
Copy link
Contributor Author

@nattthebear Good to know. But, IMHO, it shouldn't apply in case of commonjs.

@fatcerberus
Copy link

Shouldn't apply to ESM either. A script executed as type=module in a compliant engine gets its own scope regardless of whether it has any imports or exports. But I suspect this behavior exists in TS for backwards compatibility and can't easily be changed now. It's definitely a footgun, regardless.

@nattthebear
Copy link

It's typescript's solution to the Michael Jackson problem. If a script has neither exports nor imports, tsc assumes that it's not part of whatever module system you've specified in your compiler options. You can see this on the playground which is set to AMD: The right side output pane is just plain script lines, until you add an import or export on the left, at which point the entire thing gets an AMD wrapping.

https://www.typescriptlang.org/play/#src=import%20a%20from%20b

I see how it can be an issue, but it's not that likely to come up: You'd need a module with no exports, so it was imported only for side effects, and no imports, so those side effects must only be on the global object and anything reachable from there.

@fatcerberus
Copy link

Even if you import 'side-effect-file'; it still gets its own scope—you have to assign stuff to globalThis manually if you want it to be global. Likewise if the host you’re targeting is all-modules-all-the-time (like my own miniSphere) and your program only consists of a single file.

It doesn’t actually solve the Michael Jackson problem IMO because the syntactically-distinguishable modules proposal was rejected; the bare file will still be treated as a module at runtime if the host decides it wants to run code as modules, and this will be out of the user’s control. And at that point TypeScript’s idea of what your code is doing is different from what will actually happen at runtime. Hence the footgun.

Would be nice if we could disable this, at least.

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jun 9, 2019

@fatcerberus If you specify isolatedModules you will get an error in files that do not contain any exports/import. The very useful message:

Cannot compile namespaces when the '--isolatedModules' flag is provided.

Although isolatedModules might bring with it other unwanted behavior (ex this )

@fatcerberus
Copy link

fatcerberus commented Jun 9, 2019

--isolatedModules prevents type checking across files (it’s basically Transpiler Mode) from what I understand, so that’s no good.

From @weswigham in that same thread:

isolated modules compiles each file individually without the types of the files it depends on

That error message makes no sense, though. Namespaces and modules are very different things—the documentation even goes out of its way to stress it IIRC

Thanks though, I appreciate it.

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jun 9, 2019

@fatcerberus I agree about the error message being horrible, but at least it's a compile time error for something that would end up a run-time error

I am not sure about what the 'individual' part means though. I think that might refer just to the emit part not the type checking. From my testing imported modules are checked as expected with isolatedModules.

I would love some actual documentation on what isolatedModules actually does and the limitations, the current docs (Transpile each file as a separate module (similar to “ts.transpileModule”)) are not particularly clear

@fatcerberus
Copy link

fatcerberus commented Jun 9, 2019

Well this looks like bad news already:
image

As for ts.transpileModule - I've actually used it in the past, and it doesn't pull types from imports. It doesn't seem to be honored at all if I put it in my tsconfig, though.

@weswigham Could you come here and explain exactly what --isolatedModules does

@dragomirtitian
Copy link
Contributor

@fatcerberus But I think that is just about emit phase. I think the emit can't depend on types in other files. Running the command line compiler does the type checking as expected, at least from what I tested:

{
  "compilerOptions": {
    "isolatedModules": true,
    "module": "commonjs"
  }
}
// a.ts
export class MyStuff {
    x: string = "";
}
export function doStuff(cmd: MyStuff) { }

//b.ts
import { doStuff, MyStuff } from './a'
import { a } from "./aa" // error

doStuff(new MyStuff())
doStuff(""); // error
new MyStuff().x
new MyStuff().xx // error

Output:

b.ts:2:19 - error TS2307: Cannot find module './aa'.

2 import { a } from "./aa" // error
                    ~~~~~~

b.ts:5:9 - error TS2345: Argument of type '""' is not assignable to parameter of type 'MyStuff'.

5 doStuff(""); // error
          ~~

b.ts:7:15 - error TS2339: Property 'xx' does not exist on type 'MyStuff'.

7 new MyStuff().xx // error

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Jun 13, 2019
@myshov
Copy link

myshov commented Oct 2, 2019

connected to #18232

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

7 participants