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

Implement TypeScript namespace support #9785

Closed
wants to merge 2 commits into from

Conversation

@Wolvereness
Copy link
Contributor

commented Mar 28, 2019

EDIT(@nicolo-ribaudo) Merged at 0a98814

Q                       A
Fixed Issues? Fixes #8244, fixes #10038
Patch: Bug Fix?
Major: Breaking Change? Non-breaking
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link babel/website#2038
Any Dependency Changes?
License MIT

This adds basic namespace support for TypeScript in babel. Currently, it's an outright error, so this only adds new functionality where a project would otherwise outright fail to run through babel. I recommend adding some type of feature gate for this functionality, until it has been used at-large for some period of time.

The implementation may be best explained by comparing the canonical example that is added as a test case (in out).

Three known limitations (ordered by increasing complexity of respective solution):

  • Sharing the name of the directly enclosing namespace. This requires using a unique identifier as the parameter, which subsequently limits how a project using babel can interact with their namespaces. Making a dynamic solution, one that uses a unique identifier only when necessary, requires a similar solution to the next item. There is test coverage for this.

  • Exporting mutable members from inside a namespace. This requires a much more comprehensive depth-first visitor that does extensive scope checking, or an independent state machine to track transformed namespace nodes. There is test coverage for this.

  • Scope sharing. In image form: Scope Sharing Link. This would require a greater complexity than the mutable member limitation. In addition, a fully correct version requires having a full type-model spanning every file, effectively impossible using babel's model.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10893/

@amaranth

This comment has been minimized.

Copy link

commented Apr 6, 2019

This doesn't correctly handle enums inside namespaces. You end up with invalid syntax.

Input:

export namespace A {
    export enum B {
        A = 1,
        B = 2,
        C = 3,
    }
}

Output:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.A = void 0;
var A;
exports.A = A;

(function (A) {
  export var B;

  (function (B) {
    B[B["A"] = 1] = "A";
    B[B["B"] = 2] = "B";
    B[B["C"] = 3] = "C";
  })(B || (B = {}));
})(A || (exports.A = A = {}));

Config:

{
    "presets": [
        "@babel/env",
        "@babel/typescript"
    ],
    "plugins": [
        "@babel/proposal-class-properties",
        "@babel/proposal-object-rest-spread"
    ]
}

@Wolvereness Wolvereness force-pushed the velocityorg:namespace branch 2 times, most recently from 3a2e9fe to 62f2195 Apr 11, 2019

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I addressed the issue that @amaranth brought up. This also meant I added a few more test cases, like multiple namespaces only declaring themselves once, and sharing the name of an enum wont redeclare it.

The incremental change and a minor name change.

I squashed the commits, rebased to current master, and pushed it to this PR.

@Wolvereness Wolvereness force-pushed the velocityorg:namespace branch from 62f2195 to 9915c4f Apr 12, 2019

@Wolvereness

This comment has been minimized.

@ChuckJonas

This comment has been minimized.

Copy link

commented Apr 21, 2019

Would love to see this merged! I have a couple projects relying on this feature. I've already built this branch and linked it to my projects to confirm it work for my use cases.

@nicolo-ribaudo nicolo-ribaudo self-requested a review Apr 26, 2019

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Could this get a bit of feedback for the status? I know @martpie @TrySound and @hzoo were giving feedback on the issue and may have some insight for this PR.

@nicolo-ribaudo
Copy link
Member

left a comment

Hi @Wolvereness, thank you for working on this feature.

We have not been implementing namespaces in Babel for two reasons:

  1. As you pointed out, there is a big number of caveats. Even if it is not always possible, we try to match the spec closely: that's why, for example, we are testing our parser agains the test262 test suite. Implementing namespaces exactly as TS does is currently impossible because of the cross-file merging but, as you pointed out, even ignoring it leaves an high implementation complexity.
  2. TypeScript namespaces are pretty much considered derpecated: as TypeScript's PM wrote, there are better alternatives for namespaces. Also, there is a reccomended rule in TSLint to disallow them.

I'll discuss about this PR with the other maintainers during next meeting (it's in about a week, we'll publish the notes right after).

Possible approaches I can think of are:

  • Keeps things as-is;
  • Implement namespaces as it's done in this PR, even if it has all those known limitations;
  • Only accept type-only namespaces, which can be safely removed without affecting any other code.

Anyway, in the meanwhile I suggest publishing the namespaces transformer to npm as a separate plugin. If people run it before the @babel/plugin-typescript plugin (or preset), namespaces will be transpiled before that it can throw about them not being supported.

This PR also fixes an actual bug, and it would be ideal if you (or anyone else) could open a separate PR which only fixes it, since it is far less controversial than adding namespaces support.

@nicolo-ribaudo nicolo-ribaudo referenced this pull request May 13, 2019
0 of 3 tasks complete
@ChuckJonas

This comment has been minimized.

Copy link

commented May 13, 2019

@nicolo-ribaudo namespaces ARE NOT deprecated (or is there any plan to).

microsoft/TypeScript#30994

I also believe the ts-lint rule is referring to the old legacy global namespace (I could be wrong). There are many valid reasons to use namespaces for code organization.

Really the only reason NOT to use them at this point is:

A: if you are concerned with only writing ts code that conforms to ES6 (but with types)
B: If you want to use babel

By saying "Were not going to support this thing because it's basically legacy", you are essentially making that decision for the typescript team. This also causes fragmentation in the community and make the build process even more fragile/confusing.

If you're not going to support it, because it's too hard or complex to implement, I understand that. But please stop telling people that it's a "legacy feature".

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

  1. As you pointed out, there is a big number of caveats. Even if it is not always possible, we try to match the spec closely: that's why, for example, we are testing our parser agains the test262 test suite. Implementing namespaces exactly as TS does is currently impossible because of the cross-file merging but, as you pointed out, even ignoring it leaves an high implementation complexity.

I believe that the caveats would be uncommon as a limitation for a user. I included them so they would be documented and clearly benign against a common-case (the last of which I'd even put a large gamble a special case could be constructed to trip up the official compiler if types aren't declared fully). As implemented, this should satisfy the overwhelming majority of how namespaces are used. If there is a caveat not listed, then it needs to be brought to my attention.

I assumed Babel isn't taking an ideological stance, and more one from how developer time is diverted. With this PR, the time has been put in, and namespaces can be supported by Babel on a best-effort basis, similar to the rest of the typescript implementation.

  1. TypeScript namespaces are pretty much considered derpecated: as TypeScript's PM wrote, there are better alternatives for namespaces. Also, there is a reccomended rule in TSLint to disallow them.

microsoft/TypeScript#30994

Possible approaches I can think of are:

  • Keeps things as-is;
  • Implement namespaces as it's done in this PR, even if it has all those known limitations;
  • Only accept type-only namespaces, which can be safely removed without affecting any other code.

Type-only namespaces are (should-be) marked with declare, and are already removed when marked as such. https://github.com/babel/babel/tree/master/packages/babel-plugin-transform-typescript/test/fixtures/declarations/erased

This PR also fixes an actual bug, and it would be ideal if you (or anyone else) could open a separate PR which only fixes it, since it is far less controversial than adding namespaces support.

I'm not sure this is an actual bug. I asked about it on the slack and was met with radio silence. The behavior of whether or not something is in the scope doesn't seem to matter while the TS plugin runs (it doesn't use scope), and similarly there's a different expectation that an enum would get handled as a class would somewhere else.

Or to rephrase, I think the only observable behavior of it relies on this PR. Also, my interest is rather specific to supporting namespaces, so me putting in the effort of another PR is contingent on the outlook for this PR.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Thanks for linking microsoft/TypeScript#30994. I definetly had a wrong understanding of the current namespace status.


I was playing with this PR on a repl which supports TS and noticed this:

// Throws
namespace Bar {
  export let a = 2;
  a = 3;
}

// Does not throw
namespace Foo {
  export function y() {}
  y = 3;
}

is it expected?

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

It's not valid typescript. That's a type-error, something that babel shouldn't be expected to handle.
image

Edit: There are actually quite a few ways to use this implementation of namespaces poorly, but they all (sans the last limitation I explained in the description) rely on feeding in invalid typescript.

@nicolo-ribaudo
Copy link
Member

left a comment

Sharing the name of the directly enclosing namespace. This requires using a unique identifier as the parameter, which subsequently limits how a project using babel can interact with their namespaces.

I don't fully understand this restriction. Wouldn't it be possible to transpile namespaces like this, to avioid the naming conflict?

namespace A {
  export function A() {}
}
let A;

(function (_A) { // _A is generated by path.scope.generateUidIdentifier
  function A() {}
  _A.A = A;
})(A || (A = {}));
@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Sharing the name of the directly enclosing namespace. This requires using a unique identifier as the parameter, which subsequently limits how a project using babel can interact with their namespaces.

I don't fully understand this restriction. Wouldn't it be possible to transpile namespaces like this, to avioid the naming conflict?

Solving this selectively makes it two-pass. One for figuring out if we should use a unique identifier, and the second for the transforms.

We could have it always use an assumed-unique identifier. This creates its own caveat to be at least slightly divergent from an edge (out-of-specification) case in typescript if something reassigns the identifier representing the namespace itself post-initialization. It also creates another caveat; (actual question from me, not hypothetical) does the process for generating a unique identifier consider that it will be for a sub-scope? Those aren't in the scope for the parent, and deciding something (provably) unique requires it to have been inspected already.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 16, 2019

It should generate an ID which is unique in the whole file. I'm not 100% sure though.
We only need to generate the UUID for internal usage in the namespace function (and we can always to that); the external identifier shall remain the same.

This creates its own caveat to be at least slightly divergent from an edge case in typescript if something reassigns the identifier representing the namespace itself post-initialization.

Can you share an example?


EDIT: Another solution would be to make our scope tracker treat namespaces as functions.

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

This creates its own caveat to be at least slightly divergent from an edge case in typescript if something reassigns the identifier representing the namespace itself post-initialization.

Can you share an example?

I was setting it up and realized it's actually going to hit a type error. I'm going to assume now what I was thinking of isn't really possible, but I'll do a mental proof sometime this weekend to assure myself, before I write the UUID change.

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

It should generate an ID which is unique in the whole file. I'm not 100% sure though.

I actually found a bug testing this PR, not that it's particularly relevant. enum values are not unique, but everything else appears to be. #9997

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Namespace name clashes are no longer an issue.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Hi all 👋 We talked about this PR during the last meeting (notes).

The outcome was:

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

I'm very glad to hear that!

  • Is there an example of a feature flag that I could mimic, or will someone else be working on that?

  • The meeting notes mention making sure the exceptions are user friendly. Should I add the URL to the documentation for the non-const export exception? I would assume an exception for encountering a namespace outside of the feature flag may also have the URL.

  • I'm probably a good candidate to write the PR for https://github.com/babel/website/blob/master/docs/plugin-transform-typescript.md. I can work on that later in the week.

@danez

This comment has been minimized.

Copy link
Member

commented May 25, 2019

  • Is there an example of a feature flag that I could mimic, or will someone else be working on that?

It is basically the same as this options: https://babeljs.io/docs/en/babel-plugin-transform-typescript#options (jsxPragma for example)
If you have time, it would be nice if you could add an option. Otherwise it's also fine and we can do it. Just let us know.

  • The meeting notes mention making sure the exceptions are user friendly. Should I add the URL to the documentation for the non-const export exception? I would assume an exception for encountering a namespace outside of the feature flag may also have the URL.

Adding URLs sounds good. Our concern was that users who run into this limitations would think it is a bug and report it and a good error message could clarify that these cases are unsupported.

Thanks for all the effort.

path.replaceWith(getDeclaration(t, name));
path.scope.registerDeclaration(path.parentPath);
} else {
path.parentPath.replaceWith(value);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 26, 2019

Member

I noticed that exported namespaces are not exported if their original declaration isn't.

e.g. In

var foo = {};

export namespace foo { }

foo isn't exported. This PR correctly handles it; do we have a test for this behavior?

This comment has been minimized.

Copy link
@Wolvereness

Wolvereness May 28, 2019

Author Contributor

It's not actually valid typescript, but for some reason typescript wont fuss if the namespace itself is empty.

So, I'm a bit confused, what behavior should we be testing?

This comment has been minimized.

Copy link
@amaranth

amaranth May 28, 2019

I believe TypeScript ignores this because it doesn't emit anything for an empty namespace. That logic likely accidentally makes it ignore the type error as well.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 28, 2019

Member

Oh ok; maybe typescript accepts it because it is considered as a type-only namespace?

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Experimental flag added: 4d447b3

Better error message for non-const: 6d5df43

@Wolvereness Wolvereness force-pushed the velocityorg:namespace branch from 7ece4f1 to 23c764b May 29, 2019

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Does thir PR fix #10038?

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Does thir PR fix #10038?

Added as a fixture (so, not a full-babel transform), and this is the output (I'm not sure if it's correct):

let src;

(function (_src) {
  let ns1;

  (function (_ns) {
    class foo {}

    _ns.foo = foo;
  })(ns1 || (ns1 = _src.ns1 || (_src.ns1 = {})));

  let ns2;

  (function (_ns2) {
    class foo {}

    _ns2.foo = foo;
  })(ns2 || (ns2 = _src.ns2 || (_src.ns2 = {})));
})(src || (src = {}));
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Yeah, it seems correct. In the issue it didn't even produce an output 😛. Could you commit it?

so, not a full-babel transform

What do you mean?

@Wolvereness

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

so, not a full-babel transform

What do you mean?

Fixtures are unit-tests. An end-user would rarely need only the plugin that the fixtures test. For example, the typescript-supporting repl has a bit more output. It also changes the use of underscores.

"use strict";

function _instanceof(left, right) { if (right != null && typeof Symbol !== "undefined" && right[Symbol.hasInstance]) { return right[Symbol.hasInstance](left); } else { return left instanceof right; } }

function _classCallCheck(instance, Constructor) { if (!_instanceof(instance, Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

// typescript
var src;

(function (src) {
  var ns1;

  (function (ns1) {
    var foo = function foo() {
      _classCallCheck(this, foo);
    };

    ns1.foo = foo;
  })(src.ns1 = ns1 || (ns1 = {}));

  var ns2;

  (function (ns2) {
    var foo = function foo() {
      _classCallCheck(this, foo);
    };

    ns2.foo = foo;
  })(src.ns2 = ns2 || (ns2 = {}));
})(src || (src = {}));
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Oh I see; the repl output is different because there are the es2015 and stage-2 presets enabled

nicolo-ribaudo added a commit that referenced this pull request Jun 30, 2019
Implement TypeScript namespace support (#9785)
* Add module tests for typescript namespace transform

Fixes #8244, fixes #10038
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

Merged at 0a98814

@nicolo-ribaudo nicolo-ribaudo added this to the v7.5.0 milestone Jun 30, 2019

benjamn added a commit to meteor/babel that referenced this pull request Jul 6, 2019
Use actual TypeScript instead of @babel/preset-typescript.
Babel's TypeScript implementation has a few unfortunate caveats:
https://babeljs.io/docs/en/babel-plugin-transform-typescript#caveats

Most notably, the lack of *full* support for namespaces is painful:
babel/babel#8244
babel/babel#9785

By precompiling TypeScript code with the actual TypeScript compiler, we
can support features like namespaces without relying on Babel. Of course,
Babel still handles everything after TypeScript syntax has been removed.
benjamn added a commit to meteor/babel that referenced this pull request Jul 6, 2019
Use actual TypeScript instead of @babel/preset-typescript. (#25)
* Use actual TypeScript instead of @babel/preset-typescript.

Babel's TypeScript implementation has a few unfortunate caveats:
https://babeljs.io/docs/en/babel-plugin-transform-typescript#caveats

Most notably, the lack of *full* support for namespaces is painful:
babel/babel#8244
babel/babel#9785

By precompiling TypeScript code with the actual TypeScript compiler, we
can support features like namespaces without relying on Babel. Of course,
Babel still handles everything after TypeScript syntax has been removed.

* Test that JSX syntax works in .tsx files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.