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

Invalid code generated with 'export module' declaration #713

Open
theRobinator opened this issue Jan 18, 2018 · 5 comments
Open

Invalid code generated with 'export module' declaration #713

theRobinator opened this issue Jan 18, 2018 · 5 comments

Comments

@theRobinator
Copy link
Contributor

Minimal reproduction follows:

test.ts

export module Foo {
    const notAnEmptyModule = 6;
}

Output: test.js

var Foo;
(function (Foo) {
    const /** @type {number} */ notAnEmptyModule = 6;
})(Foo = exports.Foo || (exports.Foo = {}));

Compiling with Closure Compiler v20180101 (the latest at the time of this writing) fails with this error:

vulcan/build/js/test.js:10: ERROR - Exports must be a statement at the top-level of a module
})(Foo = exports.Foo || (exports.Foo = {}));
                         ^^^^^^^^^^^^^^^^

As a workaround, doing the export on a separate line generates working code:

module Foo {
    const notAnEmptyModule = 6;
}
export {Foo};
@evmar
Copy link
Contributor

evmar commented Jan 18, 2018

Within Google we disallow use of namespaces ('module' is an obsolete alias for the 'namespace' keyword here I think), in part because they generate code that confuses Closure compiler. I think in particular if you start nesting module A { module B { you'll find Closure loses what things refer to.

@theRobinator
Copy link
Contributor Author

@evmar Interesting. Is there a public collection of this kind of thing that confuses the compiler? I'm currently migrating a pretty large (500K LOC) codebase to a Typescript/Closure Compiler stack and it'd be great to know about this kind of stuff. Does this tslint.json cover it?

Our existing codebase declares a lot of properties on functions, and my translation is using declaration merging with namespaces to accomplish this. I assume you use an interface for this instead?

@evmar
Copy link
Contributor

evmar commented Jan 18, 2018

We don't have it well-documented unfortunately. The actual line is always kinda vague and varies with the rules of Closure compiler.

Coincidentally, just today I learned about properties on functions in Closure syntax. Search for "hacky" on http://closuretools.blogspot.com/2017/01/common-pitfalls-when-using-typedefs-in.html . Since I only learned about this today I doubt tsickle does the right thing here, but it might accidentally work.

@evmar
Copy link
Contributor

evmar commented Jan 18, 2018

(Note that by default tsickle leaves types out everywhere, as it's more concerned with "get the compiler to accept this code" than it is about "properly express these types so that the compiler understands them". So it often is the case that things accidentally work.)

@theRobinator
Copy link
Contributor Author

Doing it using declaration merging actually does look like it works! No type comments are generated for those expressions and so the compiler accepts it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants