Enum output differences between es5 and es2015 (Declaration Merging Support)

[dallastjames]

So, I'll prefix this with the fact that this is my first foray into typescript compiler code, so if I'm just missing obvious things, I apologize in advance. 👍

I posted a bug in the ng-packagr library back in March related to an issue during compiling with declaration merging. Since it hasn't received any solutions, I figured I'd try my hand at tracking down the cause of the error. The tl;dr of that issue is that enum declaration merging with namespaces throws a Identifier {name} has already been declared error. This is the initial code based on the typescript declaration merging documentation

enum AdapterType {
    MapValue = '[Adapter Type] map value',
    MapClass = '[Adapter Type] map class'
}
namespace AdapterType {
    /**
     * Contains an ordered array of the valid AdapterType members
     */
    export const members: AdapterType[] = [AdapterType.MapClass, AdapterType.MapValue];
}

export { AdapterType };

And this is the output when compiling to es2015.

// es2015, es6, etc.
/**
 * @fileoverview added by tsickle
 * @suppress {checkTypes,extraRequire,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc
 */
/** @enum {string} */
const AdapterType = {
    MapValue: '[Adapter Type] map value',
    MapClass: '[Adapter Type] map class',
};
var AdapterType;
(function (AdapterType) {
    AdapterType.members = [AdapterType.MapClass, AdapterType.MapValue];
})(AdapterType || (AdapterType = {}));
export { AdapterType };

You'll notice that the enum is declared initially with the const keyword, which means that the code that follows is invalid since you can't redeclare a const. The es5 version of this code that gets output uses the var keyword initially, which allows the build to continue successfully.

// es5
/**
 * @fileoverview added by tsickle
 * @suppress {checkTypes,extraRequire,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc
 */
/** @enum {string} */
var AdapterType = {
    MapValue: '[Adapter Type] map value',
    MapClass: '[Adapter Type] map class',
};
var AdapterType;
(function (AdapterType) {
    AdapterType.members = [AdapterType.MapClass, AdapterType.MapValue];
})(AdapterType || (AdapterType = {}));
export { AdapterType };

As far as I know, the output of enums as const is correct starting in es2015 since that's when those keywords were introduced, however, by doing this, it prevents compilation when using declaration merging. When running this enum file through tsc and ngc, in both cases the generated output used the var keyword regardless of target.

So, my question is, does tsickle generate enums as a const for the closure compiler for a specific reason? (again, forgive my ignorance in the compiler world if this is an obvious thing). If not, I'd love to open a discussion of what could be done to better support typescript declaration merging.

[evmar]

Nice diagnosis. I think this is just a bug. I am surprised it's legal to declare var AdapterType twice. (Ideally we'd produce a single const AdapterType and never overwrite it, but it might be hard to convince TypeScript to produce this.)

[mprobst]

You can duplicate "var" declarations in a scope in JavaScript. Odd but legal. tsickle customizes enum emit to match Closure Compiler enums, but obviously doesn't handle re-opening enums. I'm not sure how we'd generally support that though. E.g. if the enum is reopened in a different file, or the namespace comes first, could we still emit something that Closure thinks is correct? Maybe we should emit an error here?

[dallastjames]

So, I dug into closure to try to better understand how it works with enums and as far as I can tell, the closure compiler does handle re-opening the enum ok (if you take the es5 output from the OP and paste it into the closure compiler it does return valid code without any errors.). While I love the concept of declaration merging in typescript, it doesn't translate over to closure compatible js very well and I think this is a great example of how it falls short. From what I understand, an enum for closure is meant to be a constant object that has specific data values set when it is created and should remain unchanged after initialization. For that reason, I think emitting an error may make more sense logically, even if the compiler is able to handle re-opening an enum correctly.

An alternative to the declaration merging above would be something like this, which achieves the same result in TS while maintaining valid transpiled code.

enum EAdapterType {
    MapValue = '[Adapter Type] map value',
    MapClass = '[Adapter Type] map class'
}

type AdapterType = EAdapterType;

namespace AdapterType {
    export const MapValue = EAdapterType.MapValue;
    export const MapClass = EAdapterType.MapClass;
    /**
     * Contains an ordered array of the valid AdapterType members
     */
    export const members: AdapterType[] = [AdapterType.MapClass, AdapterType.MapValue];
}

export { AdapterType };

which transpiles to

/** @enum {string} */
const EAdapterType = {
    Value: 'value',
    Class: 'class',
};
var AdapterType;
(function (AdapterType) {
    AdapterType.Value = EAdapterType.Value;
    AdapterType.Class = EAdapterType.Class;
    AdapterType.members = [AdapterType.Class, AdapterType.Value];
})(AdapterType || (AdapterType = {}));

[mprobst]

The goal of tsickle is to produce code that will be accepted by Closure Compiler, but ideally also understood, i.e. type checked, and code that will optimize well.

I think to that point, this code is probably a lost cause:

var AdapterType;
(function (AdapterType) {
    AdapterType.members = [AdapterType.MapClass, AdapterType.MapValue];
})(AdapterType || (AdapterType = {}));
export { AdapterType };

That is, I wouldn't expect Closure to understand that there's an AdapterType.members property. The code might end up working at runtime or not after that, given Closure Compiler's optimizations can break code it doesn't understand.

So I'm afraid I don't think this can really work in tsickle. Maybe the best we could do is emit an error for the pattern?

[ghenadiibatalski]

Any movement on this issue?

[BzenkoSergey]

any updates?

[mprobst]

As stated above, at best we'd emit an error for this, but haven't gotten around to that.

[evmar]

If you're commenting on this issue, can you also describe what sort of code you have that you're using both declaration merging and Closure compiler?

[moccaplusplus]

If you're commenting on this issue, can you also describe what sort of code you have that you're using both declaration merging and Closure compiler?

export enum Policy {
  STANDARD = 'standard',
  STANDARD_STRICT = 'standard-strict',
  IGNORE_COOKIES = 'ignoreCookies',
  NETSCAPE = 'netscape',
  DEFAULT = 'default',
  RFC_2109 = 'rfc2109',
  RFC_2965 = 'rfc2965',
  BEST_MATCH = 'best-match',
  COMPATIBILITY = 'compatibility'
}
export namespace Policy {
  export const values = Object.keys(Policy).map(e => Policy[e]);
}