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

Erroring on module namespaces #168

Closed
robpalme opened this issue Feb 9, 2025 · 3 comments
Closed

Erroring on module namespaces #168

robpalme opened this issue Feb 9, 2025 · 3 comments

Comments

@robpalme
Copy link

robpalme commented Feb 9, 2025

Recently @swc/wasm-typescript (as used by Amaro) added type-only namespace support to align with TypeScript 5.8's new --erasableTypesOnly flag.

That PR also introduced support for the legacy module keyword for namespaces which IMO did not seem desirable for Node. The module keyword for namespaces was superseded by the namespace keyword ten years ago in TS 1.5. There are various efforts to reduce usage of the legacy keyword being coordinated in the TypeScript project, including Editor strike-through feedback in TS 5.6. Potentially the module keyword will be useful in a future JS proposal. Given Node is aiming for a safe default experience, we should be minimal and avoid the future syntax conflict risk.

I checked with @RyanCavanaugh (TS Engineering Manager & author of --erasableTypesOnly) who confirmed he is highly supportive of Node preventing the use of the module keyword for namespaces and believes all modern code will be able to handle this easily. Specifically for Amaro this means:

  • For both Node compilation modes (strip-types and transform-types):
    • module namespaces should always error: module Foo {}
    • Ambient module namespaces should always error: declare module Foo {}
  • Ambient module declarations (declare module "specifier" {}) are fine and should continue to be erased and not error

Implementation

SWC is the right place to efficiently solve this. My thinking is we should request a new SWC option such as no_module_for_ts_namespace with the above semantics. The error message would say something like:

module keyword is not supported. Use namespace instead.

We learned that this will require an update to the SWC AST which currently does not preserve knowledge of whether the namespace originated via a module or namespace source keyword. So I'd like to check here that we are ok making the request to SWC.

cc @nodejs/typescript

@marco-ippolito
Copy link
Member

Sgtm, I wish there was a TS flag that excluded module so we can match the behavior exactly.

@robpalme
Copy link
Author

robpalme commented Feb 9, 2025

Thanks, Marco. I agree.

SWC feature request is here: swc-project/swc#10017

@RyanCavanaugh
Copy link

It seems like there's already a pre-pass in the Node side to detect illegal syntax (e.g. enum) that SWC parses, so it could presumably be implemented without needing anything from the SWC side. Note that declare module "foo" { needs to remain legal to support true ambient external module declarations.

From the TypeScript side, we're in general agreement that it'd be best in the long term to not support module foo { in a future major version of TypeScript (though not 6.0 as that's just around the corner). Hopefully we can ship that deprecation early enough to eradicate its usage in the wild to leave room for any future TC39 proposal (if not, we'd have to switch its interpretation based on target, which would be unfortunate). We don't intend to ship any flag to ban this since it's very obscure (and unlikely to be opted into in practice).

TL;DR TypeScript also supports this change

acutmore added a commit to bloomberg/ts-blank-space that referenced this issue Feb 12, 2025
## Context

microsoft/TypeScript#51825
nodejs/amaro#168

## Description 

This PR is a *breaking change*.
Continuing on from #32 but instead of allowing _more_
`ModuleDeclaration` AST nodes, this PR removes support for erasing
namespace declarations that use the deprecated `module` keyword.

### Still erased (no error emitted)

```ts
declare namespace N1 {}
namespace N2 {}
declare global {}
declare module "./path" {}
```

### No longer erased (triggers `onError` callback)

```ts
declare module M1 {}
module M2 {}
```

---------

Signed-off-by: Ashley Claymore <[email protected]>
Co-authored-by: Rob Palmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants