-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
--erasableSyntaxOnly
#61011
--erasableSyntaxOnly
#61011
Conversation
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
Reading #59601 (comment), do we also need to ban |
Modulo VMS restrictions, it always transforms to |
ts-blank-space and Node both say no to |
name: "erasableSyntaxOnly", | ||
type: "boolean", | ||
category: Diagnostics.Interop_Constraints, | ||
description: Diagnostics.Do_not_allow_runtime_constructs_that_are_not_part_of_ECMAScript, |
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.
JSX?
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.
Open to suggestions 🤷
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'd vote no since that would curse anyone trying to use this for "purist" purposes to not being able to use JSX at all; you're already going to get an error from Node, and in other runtimes or with a loader, it might even work just fine....
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.
Yeah, the fact that you already have to put JSX content in a .?sx
file means you can't possibly make the mistake of "not realizing enum is special" or whatever.
I thought Daniel wanted the text to say something different, which technically JSX is a non-ES runtime construct, but the phrasing would maybe get super awkward
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.
Ohh that interpretation makes a lot more sense, oops
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.
you're already going to get an error from Node
Well you could make the same argument about all of these features right?
the fact that you already have to put JSX content in a
.?sx
file
Well we permit JSX in JS files 😄
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.
Overall I think it is fine either way, just a slightly weird distinction.
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 implementation seems right to me; cross checking with other tools, I don't believe we missed anything, but it might be a good idea to add a test that shows that namespaces need to be recursively checked.
Worth re-noting for people watching this that ts-blank-space and Node.js' Amaro currently do not handle uninstantiated (type-only) namespaces: |
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
…rasableSyntaxOnly
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
…/TypeScript into erasableSyntaxOnly
This disables ALL enums, but aren't string only enums 1 to 1 mappable to vanilla JavaScript? At runtime, as far as I can tell, a string enum behaves identical to an object composed of string values. Is this incorrect? And if not, would it be undesirable to allow string enums with this flag turned on? IMO string enums are far more ergonomic than const objects in that they are both a type and an iterable. Sorry if this is irrelevant and the ship has sailed. Just thought I'd ask. |
Enums are not "erasable" syntax - they need to be transformed to become valid Javascript. An enum like this: // Typescript
enum Direction {
Up = "UP",
Down = "DOWN"
} is transformed into // Javascript
var Direction;
(function (Direction) {
Direction["Up"] = "UP";
Direction["Down"] = "DOWN";
})(Direction || (Direction = {})); The flag introduced here aims at ensuring you get a type error when you write typescript syntax that is incompatible with Node's new type stripping feature (https://nodejs.org/en/learn/typescript/run-natively#running-typescript-code-with-nodejs), which enums are. (There is a new |
@alexbit-codemod We need a codemod for this so everyone can migrate away easily! |
This PR adds support for type-only/uninstantiated namespaces. i.e. namespaces that only use `type` and `interface`. ```ts export class C {} export namespace C { export type T = string; } ``` **Before:** Error. only `declare namespace ...` was allowed ❌ **Now:** Ok. namespace declaration erased ✅ --- The following cases remain unchanged: ```ts // Instantiated namespace errors namespace A { export let x = 1 } // error namespace B { ; } // error // Ambient namespace is erased declare namespace C { export let x = 1 } // ok erased declare module D { export let x = 1 } // ok erased // `module`-keyword errors module E { export let x = 1 } // error module F { export type x = number } // error ``` ## Testing Unit tests for both supported cases and unsupported cases (errors) have been added. ## Context This addition was motivated by [`--erasableSyntaxOnly`](microsoft/TypeScript#61011) and the recognition that in today's TypeScript, the only way to augment a class with types after the initial declaration is via namespaces. Whilst that use case can be solved using `declare namespace` that comes with [a hazard](https://github.com/bloomberg/ts-blank-space/blob/main/docs/unsupported_syntax.md#the-declare--hazard). The combination of `--erasableSyntaxOnly` checks and transforming uninstantiated namespaces provides a safer option. Thanks to @jakebailey for brining this to our attention microsoft/TypeScript#61011 (comment) --------- Signed-off-by: Ashley Claymore <aclaymore@bloomberg.net> Co-authored-by: Rob Palmer <rob.palmer2@gmail.com>
Should this option imply |
We discussed this heavily and settled on "no" for now, mainly because it makes our implied options complexity even worse, but also because it means those who are using this flag for reasons that aren't specifically needing the emit then have to disable some other flag (or if we made it required, be stuck). |
I'm coming here from the 5.8 announcement, looking for how to migrate from enums when using A popular solution that I see is using an // you can also use number literals
const MyEnum = {
Foo: "Foo",
Bar: "Bar",
} as const; This is usually accompanied with This lets you use
I don't really care about the last point, and it's easy to use the variant name anyway (it will most likely be interned by the JS engine, so perf should not be a big concern when comparing using a number VS a short string). On the other hand, I'd like to have a way to keep nominal typing and lightweight type syntax for variants. Here is what I ended up moving to: const Foo: unique symbol = Symbol("Foo");
const Bar: unique symbol = Symbol("Bar");
const MyEnum = {
Foo,
Bar,
} as const;
type MyEnum = typeof MyEnum[keyof typeof MyEnum];
declare namespace MyEnum {
type Foo = typeof MyEnum.Foo;
type Bar = typeof MyEnum.Bar;
}
export {MyEnum}; This uses Obviously, this approach is even more inconvenient at the implementation side, but I find it more convenient on the consumer side. The implementer side complexity is less relevant if using codegen. The "record of symbols" approach addresses the complaints in this blog post about support variant annotations such as I hope this message helps others when evaluating available solutions when migrating away from TS |
Implements #59601