oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
73.29k stars 2.69k forks source link

emitDecoratorMetadata fails when interfaces are imported without `import type` #8439

Open ottomated opened 8 months ago

ottomated commented 8 months ago

What version of Bun is running?

1.0.24+6fa35839c

What platform is your computer?

Linux 6.1.72 x86_64 unknown

What steps can reproduce the bug?

  1. Initialize a new TS project
  2. Remove "verbatimModuleSyntax" from tsconfig.json
  3. Add "emitDecoratorMetadata": true and "experimentalDecorators": true to tsconfig.json
  4. Use the following code:
    
    // index.ts
    import { TestInterface } from "./interface";

function Decorator(): PropertyDecorator { return () => {}; }

class TestClass { @Decorator() test?: TestInterface; }


```ts
// interface.ts
export interface TestInterface {}
  1. Observe the following error:
    1 | (function (entry, fetcher)
    ^
    SyntaxError: Export named 'TestInterface' not found in module '<...>/interface.ts'.

What is the expected behavior?

This does not error using tsc, which instead compiles it to this:

__decorate([
    Decorator(),
    __metadata("design:type", Object)
//                            ^ note the `Object`
], TestClass.prototype, "test", void 0);

It seems like Bun wants to generate this JS output

__legacyDecorateClassTS([
  Decorator(),
  __legacyMetadataTS("design:type", TestInterface)
], TestClass.prototype, "test", undefined);

but fails because TestInterface is not a value.

Jarred-Sumner commented 8 months ago

It's sometimes unclear to Bun when something is a type vs something is a value

Bun can figure this out when bundling, but at runtime it only sees the current file.

If you change it to this, does it work?

import { type TestInterface } from "./interface";
ottomated commented 8 months ago

If you change it to this, does it work?

import { type TestInterface } from "./interface";

Yes, but that breaks a different part of the codebase when executing it (both bun and node crash in the same way when executing the compiled output, related to dependency injection). I'll try to create a repro for that but it's more complex.

ottomated commented 8 months ago

@Jarred-Sumner Found the culprit possibly? It seems related to actual classes being used (as in, TestInterface is a class).

Bun (and swc) generate:

__legacyDecorateClassTS([
  Decorator(),
  __legacyMetadataTS("design:type", typeof TestClass === "undefined" ? Object : TestClass)
], TestClass.prototype, "test", undefined);

which only works if the project is being bundled into one file. Looks like there's a bug in swc which doesn't bother to import TestClass, therefore mistakenly annotating it with Object instead (which is fair, because it's imported as type-only).

Bun might be innocent in this case (when import type is used), but there's no linting tool that can properly import type for interfaces without also import typeing classes that are used for decorators - eslint is either all or nothing.

ottomated commented 8 months ago

So I guess the solution is to go through your entire project and manually make sure all interfaces are imported with import type, but all classes are imported straight - even when they're only used as a decorated type annotation. Would prefer if Bun's behavior when a straight import { TestInterface } from './interface'; is used matched tsc - it should be possible, given bun build index.ts in the above example fails at compile-time, not runtime:

❯ bun build index.ts
1 | import { TestInterface } from "./interface";
             ^
error: No matching export in "interface.ts" for import "TestInterface"
    at <...>/index.ts:1:10
ottomated commented 8 months ago

Bun fails where swc fails here: bun build index.ts --no-bundle --target=node ->

import {
__legacyDecorateClassTS as __legacyDecorateClassTS_5488bd03878138e3,
__legacyMetadataTS as __legacyMetadataTS_d3fc24b19ddc97c5
} from "bun:wrap";
let Decorator = function() {
  return () => {
  };
};

class Class {
}
__legacyDecorateClassTS_5488bd03878138e3([
  Decorator(),
  __legacyMetadataTS_d3fc24b19ddc97c5("design:type", typeof TestClass === "undefined" ? Object : TestClass)
], Class.prototype, "test", undefined);
  1. Won't bun:wrap fail to import in node?
  2. TestClass is not correctly imported (which, again, is fair because it's a type-only import)
paperdave commented 8 months ago

yes. --no-bundle is really buggy because it basically assumes it's going to be run in bun, despite --target. i dont like the workaround but if you want to do a single file bundle for something other than debugging you can pass --external '*' (i dont know if we have an issue for this)

regardless the real thing that stands out to me is that your original snippet fails to bundle at all:

image

Where if the decorator is removed it works fine.

^ i think this case is a separate bug because this report is specifically for the runtime, where every file is transpiled separately, aka --target=bun --no-bundle

TSC transpiles this by simply knowing what the import is and it can inline Object or Number or whatever the typedef is. this isnt really an option for us.

one idea would be is for when the decorator type comes from an imported value, we emit an import like this:

import { other_bindings_if_exist } from "./interface";
import * as _temp from "./interface";

then the decorator call is emitted using the property access

__decorate([
    Decorator(),
    __metadata("design:type", typeof _temp.TestInterface === 'function' ? _temp.TestInterface : Object)
], TestClass.prototype, "test", void 0);

@dylan-conway what do you think about this.