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

--erasableSyntaxOnly #61011

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

RyanCavanaugh
Copy link
Member

Implements #59601

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 22, 2025
@typescript-bot
Copy link
Collaborator

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.

@jakebailey
Copy link
Member

Reading #59601 (comment), do we also need to ban import foo = x.y? Or is that erasable?

@RyanCavanaugh
Copy link
Member Author

do we also need to ban import foo = x.y? Or is that erasable?

Modulo VMS restrictions, it always transforms to const foo = x.y, but I would not categorize this as an erasure

@jakebailey
Copy link
Member

ts-blank-space and Node both say no to import Foo = Bar.Baz, so that mostly settles it.

name: "erasableSyntaxOnly",
type: "boolean",
category: Diagnostics.Interop_Constraints,
description: Diagnostics.Do_not_allow_runtime_constructs_that_are_not_part_of_ECMAScript,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSX?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions 🤷

Copy link
Member

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....

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member

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 😄

Copy link
Member

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.

Copy link
Member

@jakebailey jakebailey left a 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.

class PrivateClass {

}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}
namespace IllegalBecauseNestedInstantiated {
namespace Nested {
export const m = 1;
}
}

Might be good to explicitly note that nesting needs to be checked too in case third parties read our tests and don't realize their "contains values" check has to be recursive.

namespace NotInstantiated {
export interface JustAType { }
export type ATypeInANamespace = {};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
namespace Nested {
export type ATypeInANamespace = {};
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be updated to copy the state of the other file now that it changed?

@jakebailey
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

4 participants