-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Allow export default const enum, export default enum, export default … #18628
New issue
Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “No 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? No Sign in to your account
Allow export default const enum, export default enum, export default … #18628
Conversation
Hmm it seems to pass here, but I'm getting...?
|
@@ -1310,7 +1310,11 @@ namespace ts { | |||
nextToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nextTokenCanFollowDefaultKeyword doesn't really seem like an accurate name considering it's looking at multiple? Same with nextTokenCanFollowModifier. Would something like declarationCanFollowExportDefault
be more appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is fine since it doesn't take a declaration
as input, it just goes to the next token and looks at its kind and maybe does some minimal lookahead.
export default const a = 'a'; Still get error in tsc 2.6.0-dev.20170921 |
@Andy-MS can you please review this change.. we need to make sure all LS features work as expected with this change.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay
src/compiler/parser.ts
Outdated
@@ -1310,7 +1310,11 @@ namespace ts { | |||
nextToken(); | |||
return token() === SyntaxKind.ClassKeyword || token() === SyntaxKind.FunctionKeyword || | |||
token() === SyntaxKind.InterfaceKeyword || | |||
token() === SyntaxKind.EnumKeyword || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is big enough now to deserve a switch statement.
src/compiler/parser.ts
Outdated
(token() === SyntaxKind.AbstractKeyword && lookAhead(nextTokenIsClassKeywordOnSameLine)) || | ||
(token() === SyntaxKind.DeclareKeyword && lookAhead(nextTokenIsClassKeywordOnSameLine)) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also test export default declare namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should probably allow the old module
keyword here as equivalent with namespace
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about export default declare abstract class
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be supported too.
var A; | ||
(function (A) { | ||
A[A["FOO"] = 0] = "FOO"; | ||
})(A = exports.A || (exports.A = {})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an emitter bug -- It should be writing to exports.default
.
return 0xC0FFEE; | ||
} | ||
N.foo = foo; | ||
})(N = exports.N || (exports.N = {})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same emitter bug here -- should write to exports.default
, not exports.N
.
e309554
to
39b9cc3
Compare
@k8w See https://esdiscuss.org/topic/why-is-export-default-var-a-1-invalid-syntax for why that doesn't make sense. |
My turn to apologize for the delay; sorry. I rewrote Currently the enum transformer generates something like In general, the behavior of ts.ts:visitEnumDeclaration (and visitModuleDeclaration) seems weird. Doing the The latest commit transforms as follows for ES6, with the further changes for commonjs etc being handled by the respective transformers. export default enum A { FOO = 2 };
// should be transformed into
var A = A || {};
export default A;
(function (A) {
A[A["FOO"] = 2] = "FOO";
})(A);
export enum A { FOO = 2 };
// should be transformed into
export var A = A || {};
(function (A) {
A[A["FOO"] = 2] = "FOO";
})(A);
namespace ts {
export enum A { FOO = 2 };
}
// should be transformed into
var ts = ts || {};
(function (ts) {
var A = ts.A || {};
ts.A = A;
(function (A) {
A[A["FOO"] = 2] = "FOO";
})(A);
})(ts);
It's very much possible I've missed some slight semantic difference... I'd appreciate feedback on my approach before I clean up the remaining errors/problems. |
"use strict"; | ||
exports.__esModule = true; | ||
// https://github.com/Microsoft/TypeScript/issues/3792 | ||
var N = exports.N || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exports.N is a bit awkward here, but AFAIK should always be undefined, as things like the following aren't allowed?
export default class N {}
export class N {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above regarding enum
, in this case N
should only ever be a local binding inside the module. This should read:
var N = N || {};
exports["default"] = N;
(function (N) {
function foo() {
return 0xC0FFEE;
}
N.foo = foo;
})(N);
We also should consider allowing a "nameless" namespace:
// ts
export default namespace {
export function foo() {
return 0xC0FFEE;
}
}
// js
var _a = {};
exports["default"] = _a;
(function (_a) {
function foo() {
return 0xC0FFEE;
}
_a.foo = foo;
})(_a);
// alternatively
exports["default"] = (function () {
var _a = {};
function foo() {
return 0xC0FFEE;
}
_a.foo = foo;
return _a;
})();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, I'll get started on it. Would a nameless namespace be equivalent to a "namespace expression"? In any case I see some issues when module=es6:
export default namespace {
export const a = 2;
}
// namespace merge
export default namespace {
export const b = 2;
}
// requires generated variable name:
var _gen0 = {}
export default _gen0;
(function (_gen0) {
_gen0.a = 2;
})(_gen0);
(function (_gen0) {
_gen0.b = 2;
})(_gen0);
As anonymous interfaces aren't allowed either currently, maybe this would be better handled as a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point, and perhaps we should hold off on nameless enum/namespace for now until the team has had an opportunity to discuss. My intuition is that you would not be able to merge with a nameless default namespace.
@DanielRosenwasser, @mhegazy: any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the above feedback and add tests for this emit output in the following contexts:
--target es5 --module commonjs
--target es5 --module amd
--target es5 --module system
--target es5 --module es2015
--target es2015 --module commonjs
--target es2015 --module amd
--target es2015 --module system
--target es2015 --module es2015
Also please add tests that verify declaration file (.d.ts) output.
src/compiler/parser.ts
Outdated
return lookAhead(nextTokenIsClassKeywordOnSameLine); | ||
case SyntaxKind.DeclareKeyword: | ||
// "export default declare class" and "export default declare abstract class" are both valid | ||
return lookAhead(nextTokenIsClassKeywordOnSameLine) || lookAhead(nextTokenIsAbstractKeywordOnSameLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookahead is not free, so I would recommend combining the two operations into a single lookahead. Also, you really want to do 2 token lookahead here for abstract
:
default declare class
- OKdefault declare abstract
- Not OK (especially if its followed byfunction
, etc.)default declare abstract class
- OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about these:
default declare function
default declare async function
(this syntax is invalid, however we report a more detailed error during grammar check)default declare namespace
default declare module
default declare enum
default declare const enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClassDeclaration and FunctionDeclaration exported as default
can elide the declaration name. Are we intending to support that for InterfaceDeclaration, EnumDeclaration, and ModuleDeclaration as well?
src/compiler/transformers/ts.ts
Outdated
&& moduleKind !== ModuleKind.ESNext | ||
&& moduleKind !== ModuleKind.System); | ||
} | ||
// function hasNamespaceQualifiedExportName(node: Node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid commented code.
"use strict"; | ||
exports.__esModule = true; | ||
// https://github.com/Microsoft/TypeScript/issues/3792 | ||
var A = exports.A || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we intend for enum
to work similar to class
and function
, then this is not the correct emit. Here you are exporting A
as both A
and default
, where for a class or function it only exports default
while the declaration name is a local binding only within the module.
With your new proposed emit this should instead be:
var A = A || {};
exports["default"] = A;
(function (A) {
A[A["FOO"] = 0] = "FOO";
})(A);
If we plan to allow elision of the enum name (like you can with ES6 classes and functions), then you might emit:
// ts
export default enum {
FOO = 1;
}
// js
var _a = {};
exports["default"] = _a;
(function (_a) {
_a[_a["FOO"] = 0] = "FOO";
})(_a);
// or, alternatively:
exports["default"] = (function () {
var _a = {};
_a[_a["FOO"] = 0] = "FOO";
return _a;
})();
"use strict"; | ||
exports.__esModule = true; | ||
// https://github.com/Microsoft/TypeScript/issues/3792 | ||
var N = exports.N || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above regarding enum
, in this case N
should only ever be a local binding inside the module. This should read:
var N = N || {};
exports["default"] = N;
(function (N) {
function foo() {
return 0xC0FFEE;
}
N.foo = foo;
})(N);
We also should consider allowing a "nameless" namespace:
// ts
export default namespace {
export function foo() {
return 0xC0FFEE;
}
}
// js
var _a = {};
exports["default"] = _a;
(function (_a) {
function foo() {
return 0xC0FFEE;
}
_a.foo = foo;
})(_a);
// alternatively
exports["default"] = (function () {
var _a = {};
function foo() {
return 0xC0FFEE;
}
_a.foo = foo;
return _a;
})();
I think we need to discuss the nameless declarations in the design meeting first. |
29a8fad
to
a0e3747
Compare
I've overwritten the previous commits with two new ones. The first one includes all new tests and changes to parser.ts. The changed files are limited to the new tests and 2 others. The second commit changes the emit in ts.ts, which results in a lot of changed test results. I've closely reviewed the changes in my new tests and looked at a couple of the others at random. There's still a weird exception for outputting comments when in system module in ts.ts. https://github.com/Microsoft/TypeScript/pull/18628/files#diff-c0d0e8b1528663b6cff3bb893150cec3R2873 This seems like it would better be properly handled in transformers/system.ts I had to change my proposed transform slightly: export enum A { FOO = 2 };
// should be transformed into
export var A = A || {};
(function (A) {
A[A["FOO"] = 2] = "FOO";
})(A);
const usage = A.FOO the above doesn't work, because binder.ts creates both an export and a local symbol for the enum, and the exports.A = {}
(function (A) {
A[A["FOO"] = 2] = "FOO";
})(A);
const usage = A.FOO // breaks because no local is defined. Forcing it to always use the exported version on emit breaks for export default, where it uses the non-existent "exports.A". As far as I can tell, changing this would require significant changes in the binder so that Current ES2015 output (which is correctly transformed by the following transformers): const A = {};
export { A };
(function (A) {
A[A["FOO"] = 2] = "FOO";
})(A);
const usage = A.FOO On topic, why are some transformations delayed until onEmit? I couldn't find any hint in the source. |
case SyntaxKind.DeclareKeyword: | ||
case SyntaxKind.AsyncKeyword: | ||
case SyntaxKind.TypeKeyword: | ||
return lookAhead(nextTokenIsIdentifierOrKeywordOnSameLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rbuckton This simplified lookahead seems to be enough; see 5113249#diff-cc2127059a7624c4bd840e11d106cb81
src/compiler/parser.ts
Outdated
} | ||
return token() !== SyntaxKind.AsteriskToken && token() !== SyntaxKind.AsKeyword && token() !== SyntaxKind.OpenBraceToken && canFollowModifier(); | ||
} | ||
if (token() === SyntaxKind.DefaultKeyword) { | ||
return nextTokenCanFollowDefaultKeyword(); | ||
return nextTokenCanFollowDefaultModifier(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole parseModifiers method is very confusing...
src/compiler/binder.ts
Outdated
node.localSymbol = local; | ||
local.exportSymbol = declareSymbol(container.symbol.exports, container.symbol, node, symbolFlags, symbolExcludes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I flipped these two lines so I can access node.localSymbol in declareSymbol
so can check if two default exports do not have the same identifier. Maybe this is better handled in checker.ts?
"namespace.ts", | ||
"type.ts" | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this project test to test the .d.ts output, is there a better way? Can I suppress the .js output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to avoid adding new "projects" tests in favor test variations in compiler/conformance tests. You can set // @declaration: true
in the compiler/conformance tests to get .d.ts output.
// declaration we do not emit a leading variable declaration. To preserve the | ||
// begin/end semantics of the declararation and to properly handle exports | ||
// we wrap the leading variable declaration in a `MergeDeclarationMarker`. | ||
const mergeMarker = createMergeDeclarationMarker(statement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MergeDeclarationMarker are unnecessary if the variable is initialized immediately. I think the logic handling those in the other locations is now dead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other places that still create a MergeDeclarationMarker? If not are we able remove it completely from types.ts/factory.ts?
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
If someone OKs my proposed transform above I can rebase this (assuming there's still interest). |
Yes. There're still people, like me, waiting for it being merged. |
If I export default a const enum, it will eventually produce undefined value. const enum Status{
good = 0,
bad = -1
}
export default Status; // this can lead undefined error. when using as `Status.good` |
@banxi1988 I don't follow...? |
@NaridaL sorry, I do actually encountered the undefined error caused by export default const enum type in my project. but Haven't reproduce it in a demo project. I'll try to reproduce it with a webpack configured project. |
@NaridaL if you can get this merged up with master we can give it a fresh review. Thanks for your patience! |
…icrosoft#3792 (comment). Added corresponding tests. NB: export defaults are not yet transformed correctly.
a0e3747
to
cdc7090
Compare
cdc7090
to
59c39e2
Compare
Rebased it... In: tests/baselines/reference/transformApi/transformsCorrectly.transformAddCommentToNamespace.js The comment is being output both on the var declaration and the module statement. Could someone please clarify what the difference between an emit node and a synthesized node and when/why they should be used? Setting a flag or comment range is clearly the cause of the above issue, but it's not obvious where... it works fine in the tests I added. |
A "synthesized" node is a |
If you set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of errors in the various module-specific emit tests that need to be addressed.
setTextRange(enumStatement, node); | ||
setCommentRange(enumStatement, node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be necessary to set the comment range if you've also set the text range. Setting the text range sets the pos
and end
of enumStatement
to be that of node
, which affects both comment emit and source map emit. Setting the comment range should be redundant.
addEmitFlags(enumStatement, emitFlags); | ||
statements.push(enumStatement); | ||
|
||
// Add a DeclarationMarker for the enum to preserve trailing comments and mark | ||
// the end of the declaration. | ||
statements.push(createEndOfDeclarationMarker(node)); | ||
// statements.push(createEndOfDeclarationMarker(node)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we no longer need the end-of-declaration marker node? It's used to handled deferred exports for ES2015, CommonJS, AMD, and SystemJS. If we no longer need it I would remove this line rather than just comment it.
|
||
// (function (x) { | ||
// x[x["y"] = 0] = "y"; | ||
// ... | ||
// })(x || (x = {})); | ||
// })(x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make this change to emit behavior conditional on when it is necessary rather than always emit this way? It would significantly reduce the number of test changes in the PR.
// declaration we do not emit a leading variable declaration. To preserve the | ||
// begin/end semantics of the declararation and to properly handle exports | ||
// we wrap the leading variable declaration in a `MergeDeclarationMarker`. | ||
const mergeMarker = createMergeDeclarationMarker(statement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other places that still create a MergeDeclarationMarker? If not are we able remove it completely from types.ts/factory.ts?
@@ -4101,7 +4126,7 @@ namespace ts { | |||
|
|||
sourceFile.flags |= NodeFlags.PossiblyContainsImportMeta; | |||
} | |||
else { | |||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation here is wrong.
/** | ||
* namespace B 1 leading comment | ||
*/ | ||
var B = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B
is never exported, despite the fact it is declared as export enum B
in the test.
@@ -0,0 +1,62 @@ | |||
// https://github.com/Microsoft/TypeScript/issues/3792 | |||
// @target: es2015 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our compiler runner supports test variations for the same source. Instead of multiple nnnTargetXModuleY.ts
files you can do this instead:
// @target: es2015 | |
// @target: es2015, es5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice, but it doesn't look like it's in yet?
Error: Unknown value 'es5, es2015' for compiler option 'target'.
@@ -0,0 +1,62 @@ | |||
// https://github.com/Microsoft/TypeScript/issues/3792 | |||
// @target: es2015 | |||
// @module: amd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our compiler runner supports test variations for the same source. Instead of multiple nnnTargetXModuleY.ts
files you can do this instead:
// @module: amd | |
// @module: amd, commonjs, system, es2015 |
@@ -0,0 +1,62 @@ | |||
// https://github.com/Microsoft/TypeScript/issues/3792 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding these tests in a relevant location under tests/cases/conformance/**
as tests/cases/compiler
is extremely cluttered and unorganized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, consider adding // @declaration: true
to verify .d.ts emit and // @sourceMaps: true
to verify source-map emit.
"namespace.ts", | ||
"type.ts" | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to avoid adding new "projects" tests in favor test variations in compiler/conformance tests. You can set // @declaration: true
in the compiler/conformance tests to get .d.ts output.
Sorry for a dumb question, but does this add support for |
Closing due to lack of activity here. Thanks for the work so far - if anyone would like to pick up on this feature, please do! |
Is there an "authoritative" issue for people to subscribe to in order to be kept in the loop on this feature eventually dropping? This PR doesn't reference any issues as far as I can see and searching for export default and export default type returns 50 pages of results. |
covers - "[abstract] class" - interfaces as per microsoft/TypeScript#3792 (comment) in 2020/12/15 other constructs (microsoft/TypeScript#18628 (comment)) are not yet supported by TypeScript itself
covers - "[abstract] class" - interfaces as per microsoft/TypeScript#3792 (comment) in 2020/12/15 other constructs (microsoft/TypeScript#18628 (comment)) are not yet supported by TypeScript itself
covers - "[abstract] class" - interfaces as per microsoft/TypeScript#3792 (comment) in 2020/12/15 other constructs (microsoft/TypeScript#18628 (comment)) are not yet supported by TypeScript itself
covers - "[abstract] class" - interfaces as per microsoft/TypeScript#3792 (comment) in 2020/12/15 other constructs (microsoft/TypeScript#18628 (comment)) are not yet supported by TypeScript itself
covers - "[abstract] class" - interfaces as per microsoft/TypeScript#3792 (comment) in 2020/12/15 other constructs (microsoft/TypeScript#18628 (comment)) are not yet supported by TypeScript itself
@NaridaL why was this closed? Is there any chance of having |
…declare class
Also export default namespace.
#3792 (comment)