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

files are not scoped if they don't contain at least one export or import statement #18232

Closed
Hotell opened this issue Sep 3, 2017 · 31 comments · Fixed by #47495
Closed

files are not scoped if they don't contain at least one export or import statement #18232

Hotell opened this issue Sep 3, 2017 · 31 comments · Fixed by #47495
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: ES Modules The issue relates to import/export style module behavior Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@Hotell
Copy link

Hotell commented Sep 3, 2017

TypeScript Version: 2.5.2

Code

// a.ts
const template = document.createElement('template')
template.innerHTML = `...`
class Foo extends HTMLElement {}
customElements.define('my-foo',Foo)

// b.ts
const template = document.createElement('template')
template.innerHTML = `...`
class Bar extends HTMLElement {}
customElements.define('my-bar',Bar)

// main.ts
import './a'
import './b'

image

tsconfig:

{
  "compilerOptions": {
    "target": "es5",
    "module": "commonjs",
    "sourceMap": true,
    "outDir": "./ts-output",
    "strict": true,
    "pretty": true,
    "moduleResolution": "node"
  },
  "include": ["./src"],
  "exclude": ["node_modules"]
}

Expected behavior:
every file is isolated module, variable definitions should not be leaking, as they are private if not exported

Actual behavior:
will get TS error unless export or import is used within a.ts or b.ts

@aluanhaddad
Copy link
Contributor

That is how modules are specified in ECMAScript.

@justinfagnani
Copy link

@aluanhaddad that's not true. Files are modules if they are imported from a module, or loaded with script/type=module, and have a JavaScript mine type, that's all.

TypeScript should follow they spec there.

@Hotell
Copy link
Author

Hotell commented Sep 4, 2017

@aluanhaddad
yup, what Justin said ;)

@kitsonk
Copy link
Contributor

kitsonk commented Sep 4, 2017

ES2015 made no assertions to how modules will be loaded, only how they are to be parsed. A module without imports or exports is symantically valid though.

The loading and resolution of modules was determined by the WHATWG. Even that does not make assertions about when loading a resource determining how to load it. NodeJS has chosen to load ESModules only with the .mjs extension, where as the behaviour of browsers has followed the convention about the mime-type or script tag attribute.

So the discussion here, everyone is sort of right and wrong at the same time.

Of course the OP isn't authoring ESModules, nor using an ES Module Loader to load them. They are transpiling to CommonJS. TypeScript has to determine when something is a module that needs to be transpiled to another format and when something is just a JavaScript file with no special need to transpile to the target module format. They way at the moment that is determined is if the file contains any import or export statements. Lacking those statements, TypeScript assumes it is simply transpiling a non-module.

The easiest work around is to export undefined which results in no emit in the functional code, but does "flag" to the compiler that it should emit a module. For example the following:

console.log('I am a module');

Would emit:

console.log('I am a module');

Where as:

console.log('I am a module');

export let undefined;

Would emit:

"use strict";
exports.__esModule = true;
console.log('I am a module');

This would cause NodeJS (or any other CommonJS loader) to treat the code as a module.

@justinfagnani
Copy link

That WHATWG loader link is seriously deprecated. At the moment there is no user-land exposed loader, nor any plans to add it. The spec language for how to load modules is directly in the HTML spec now, and it does say how to load it: imports and script/type=module are the only things that determine a module.

This error is happening during type-checking, and is likely independent of the module configuration. TypeScript should adhere to the specs and treat anything imported as a module.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 4, 2017

TypeScript should adhere to the specs

Which specs would those be? Links would be appreciated.

@justinfagnani
Copy link

@DanielRosenwasser
Copy link
Member

How should TypeScript know when an arbitrary file is a module vs a global? If the language isn't aware of any importing modules for that file, then it may not do the correct thing.

Right now everything is global by default which is definitely the least optimal state of the world, but I don't think this is a straightforward problem given where we are now.

@DanielRosenwasser DanielRosenwasser added the Domain: ES Modules The issue relates to import/export style module behavior label Sep 5, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Sep 5, 2017

Right now everything is global by default which is definitely the least optimal state of the world, but I don't think this is a straightforward problem given where we are now.

Even if the compiler "modularlized" anything that was imported, that may not reflect the intent of the code, as often code written without any imports or exports is intended to be loaded in the global namespace and would be a breaking change. We have often used import 'foo'; exactly for its global side-effects, to load a 3rd party library or polyfill.

I can understand though that there can be modular code which can, as the example above, interact with the DOM but not be safe to load into the global namespace and still not have any imports or exports but that seems to be an edge case.

Could a triple slash directive to force a module be a solution? Similar to the way that some of the AMD information can be specified.

@justinfagnani
Copy link

justinfagnani commented Sep 5, 2017

Given that browsers treat files differently depending on how they're loaded, it's seems like the compiler should too. Given a set of entry points, the compiler can track how each file is imported: script, module, or require()

@kitsonk
Copy link
Contributor

kitsonk commented Sep 5, 2017

First, the compiler doesn't have visibility of all those methods of import.

Ultimately it can go both ways. A file with import or export is clearly a module. While an ES Module capable browser loading a file through import will load the file as a module, NodeJS will only eventually attempt to load an ES Module if it has the .mjs extension. In other module formats that like AMD, UMD, or CommonJS, that behaviour doesn't apply. Throw in bundlers and other transpilers and it gets even more complicated. People are authoring code both ways today.

The right thing to do in my opinion is preserve the current behaviour but give a way for developers to override that default behaviour on a per file basis (ergo the triple-slash directive).

@RyanCavanaugh
Copy link
Member

There's already a syntactic directive for turning something into a module when it has no imports/exports:

export { }

@kitsonk
Copy link
Contributor

kitsonk commented Sep 5, 2017

😊 that is a heck of a lot better than my export let undefined!

@mhegazy
Copy link
Contributor

mhegazy commented Sep 6, 2017

There is a proposal to add a pragma "use module"; to eliminate this ambiguity.
Our recommendation is to use export {}; to your file to force treating it as a module.

@mhegazy mhegazy added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Sep 6, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Sep 6, 2017

That makes sense, to follow the TC39 workaround until the pragma reaches Stage 3.

@Jessidhia
Copy link

Use export {} to force a file to be treated as a module. Giving the --isolatedModules flag will also ensure that all your files are modules, by making it a compilation error if it isn't one. (Ambient declaration files are unaffected)

@shivanshu3
Copy link
Member

The following scenario isn't covered by --isolatedModules. I have 3 files in the root of the project: foo.ts, bar.d.ts, tsconfig.json and they have the following contents:

foo.ts:

let a = baz;
export {}

bar.d.ts:

declare var baz: number;

tsconfig.json:

{
	"compilerOptions": {
		"target": "es5",
		"module": "commonjs",
		"strict": true,
		"isolatedModules": true
	}
}

The code above compiles but when I run node foo.js, it throws an error saying ReferenceError: baz is not defined

The TypeScript compiler should complain about not being able to compile bar.d.ts because it is not a module.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jan 21, 2018

@shivanshu3 that is because bar.d.ts does not declare a module scoped var. It ambiently declares a global var just as lib.es5.d.ts declares Array.

bar.d.ts states that "There exists a global variable, baz of type number."

In other words, modules, isolated or otherwise, still have access to globals declarations. Whether or not this is desirable, it is how JavaScript works.

Consider renaming bar.d.ts to bar.ts and changing its contents to

var foo: number;

in order to define the global.

Alternately, if you want foo to be scoped to a module bar, you need to write

// bar.d.ts
export declare var foo: number;

or

// bar.ts
export var foo: number;

@shivanshu3
Copy link
Member

It makes sense what you are saying. But my point is, a poor module declaration file shouldn't be able to pollute the namespace of all my modules and then produce false compilation successes.

In other words, I'm adding a suggestion to add a flag which prohibits this behavior. The modules should explicitly specify which variable declarations they want to use from the global namespace, something like this:

import { Array, Buffer } from global;

I guess I should make another GitHub issue for this?

@RyanCavanaugh
Copy link
Member

But my point is, a poor module declaration file shouldn't be able to pollute the namespace of all my modules and then produce false compilation successes.

There's no distinguishing characteristic between a "poor module" and a "good global" file. If you want to ensure that a file is a module, there's already syntax for it, export { }.

I guess I should make another GitHub issue for this?

This isn't something we'd do. There's no ES6 construct for "import from global" and we're not going to add something that looks like it's a module import but isn't.

@Smesharikable
Copy link

Wouldn't it be natural to also treat every .ts file as a module if --isolatedModules is specified, regardless of whether the file contains import/export or not?

@osyrisrblx
Copy link

I'd love to see a tsconfig.json option that compiles all of my .ts files inside of rootDir with export {}; implicitly.

Having to remember to add an empty export on files which do not export/import is one of the worst experiences with using TS.

@leefernandes
Copy link

we're not going to add something that looks like it's a module import but isn't.

Is it necessary that non-module ts files be compiled to a shared global scope? It seems preferable to have non exported files be encapsulated, regardless of export rules.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 2, 2019

Is it necessary that non-module ts files be compiled to a shared global scope?

That is the only logical conclusion to come to, as that is how JavaScript is evaluated when it isn't a module.

There is a TC39 proposal, Stage 1, that address this concern in JavaScript/ECMAScript: https://github.com/tc39/proposal-modules-pragma. I don't know how active it is and what sort of support it has, but clearly it isn't a "TypeScript" problem as much as it is a "JavaScript" problem.

@osyrisrblx
Copy link

@kitsonk I'm a little confused here. Correct me if I'm wrong, but for an environment like NodeJS, files are always modules (or at least they behave that way!).

// a.ts
const x = 1;
// b.ts
require("./a");
console.log(x);

node out/b.js results in ReferenceError: x is not defined

You could say that this is NodeJS not abiding by the JS spec, but TS has added options for stuff like this in the past. i.e. "moduleResolution": "node",

I don't really understand the reasons against adding a tsconfig.json option to assume files are modules.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 2, 2019

You could say that this is NodeJS not abiding by the JS spec

Correct. There are Scripts and Modules in the specification. Of course Node.js had its behaviour well before there were modules in the specification.

I don't really understand the reasons against adding a tsconfig.json option to assume files are modules.

My response was specifically to the suggestion of "Is it necessary that non-module ts files be compiled to a shared global scope?" The answer to that is yes. Because there is a difference between a Script and a Module in the language specification, and there are expectations about how it is interpreted. The current behaviour, IMO, is appropriate and valid, and I linked to the JavaScript proposal that this is a JavaScript problem as well.

As far as a flag, well I generally see that as being useless, because the current pattern of export {} is effective and as opt-in as a flag would be, but that isn't in itself a reason. The compelling reason in my mind is why should TypeScript come up with a unique solution, when there is a proposed solution for JavaScript and a current effective workaround.

@leefernandes
Copy link

The compelling reason in my mind is why should TypeScript come up with a unique solution

Because Typescript has a compiler which ships with many options, and it'd be appreciated.

@osyrisrblx
Copy link

osyrisrblx commented Aug 2, 2019

I think it just comes down to that this is a frustrating workflow thing that no one seems to want to fix.

I can't expect NodeJS to make non-module files share global scope. That would be a huge breaking change.

I can't expect TC39 to make a spec fix for this because they're more concerned about browser usage (where this functionality makes a bit more sense)

It seems everyone is passing the blame.

@nayeemrmn
Copy link

There's already a syntactic directive for turning something into a module when it has no imports/exports:

export { }

There is a proposal to add a pragma "use module"; to eliminate this ambiguity.
Our recommendation is to use export {}; to your file to force treating it as a module.

That makes sense, to follow the TC39 workaround until the pragma reaches Stage 3.

Browser and Node host environments specify whether something is a script or a module out-of-band (type="module", "type": module"). It's not correct to consider export {}; a TC39-given directive to make something a module when it's supposed to throw a syntax error when used in scripts; using it for inference is entirely a TypeScript invention. It's clear that tsc should have a compiler option to treat all or some source files as modules without requiring any particular syntax usage within the files.

@nayeemrmn
Copy link

nayeemrmn commented Oct 3, 2021

The option introduced in #45884 should provide a fix for this, but it doesn't work yet.

// main.ts
import "./a.js";
import "./b.js";

// a.ts
const a = 1;
console.log(a);

// b.ts
const a = 2;
console.log(a);

// package.json
{
  "type": "module"
}

//tsconfig.json
{
  "compilerOptions": {
    "module": "nodenext"
  },
  "include": ["*.ts"],
  "exclude": ["node_modules"]
}

tsc --version && tsc && node main.js

Version 4.5.0-beta
a.ts:1:7 - error TS2451: Cannot redeclare block-scoped variable 'a'.

1 const a = 1;
        ~

  b.ts:1:7
    1 const a = 2;
            ~
    'a' was also declared here.

b.ts:1:7 - error TS2451: Cannot redeclare block-scoped variable 'a'.

1 const a = 2;
        ~

  a.ts:1:7
    1 const a = 1;
            ~
    'a' was also declared here.


Found 2 errors.

Same happens if I renamed to a.mts and b.mts (shouldn't make a difference with "type": "module" but is more obviously incorrect). We now have a configuration to specify usage of ES modules out-of-band, so it should just come down to a bug fix 🤞

cc @weswigham

@weswigham
Copy link
Member

So, we've actually wanted to fix this for awhile (at least since the new jsx transforms), but our parser API is in the way - we'd have to break it to support locking in module mode parsing, because currently our parser API doesn't take compiler options of any kind. T.T

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: ES Modules The issue relates to import/export style module behavior Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

16 participants