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

[swc] Allow keepClassNames configuration, or default to true #1343

Closed
ldiqual opened this issue May 25, 2021 · 4 comments · Fixed by #1344
Closed

[swc] Allow keepClassNames configuration, or default to true #1343

ldiqual opened this issue May 25, 2021 · 4 comments · Fixed by #1344
Milestone

Comments

@ldiqual
Copy link

ldiqual commented May 25, 2021

Desired Behavior

In https://github.com/TypeStrong/ts-node/blob/main/src/transpilers/swc.ts#L57-L88 there is no reference to keepClassNames which is required for popular libraries like TypeORM to work.

Proposal is to either:

  1. Make jsc.keepClassNames = true by default
  2. Allow setting keepClassNames somehow (maybe in ts-node.compilerOptions.keepClassNames)

Is this request related to a problem?

With ts-node v10 and swc 1.2.58, running typeorm migrations result in this kind of error:

Error during migration run:
Error: Entity metadata for Entity#field was not found. Check if you specified a correct entity object and if it's connected in the connection options.
    at /Users/ldiqual/Siteline/siteline/src/metadata-builder/EntityMetadataBuilder.ts:671:23
    at Array.forEach (<anonymous>)
    at EntityMetadataBuilder.computeInverseProperties (/Users/ldiqual/Siteline/siteline/src/metadata-builder/EntityMetadataBuilder.ts:666:34)
    at /Users/ldiqual/Siteline/siteline/src/metadata-builder/EntityMetadataBuilder.ts:118:56
    at Array.forEach (<anonymous>)
    at EntityMetadataBuilder.build (/Users/ldiqual/Siteline/siteline/src/metadata-builder/EntityMetadataBuilder.ts:118:25)
    at ConnectionMetadataBuilder.buildEntityMetadatas (/Users/ldiqual/Siteline/siteline/src/connection/ConnectionMetadataBuilder.ts:66:111)
    at Connection.buildMetadatas (/Users/ldiqual/Siteline/siteline/src/connection/Connection.ts:517:59)
    at Connection.<anonymous> (/Users/ldiqual/Siteline/siteline/src/connection/Connection.ts:193:18)
    at step (/Users/ldiqual/Siteline/siteline/node_modules/typeorm/node_modules/tslib/tslib.js:143:27)

This is what keepClassNames is for: swc-project/swc#1279

@cspotcode
Copy link
Collaborator

Thank you for the detailed report.

Your proposed option 1. seems the easiest for everyone. Are there any situations where a user would want to disable keepClassNames?

If we need to implement option 2, I think we can support it like this. The example below does not work today, but the necessary code change is small.

// tsconfig.json
{
  "ts-node": {
    // This is undocumented, but transpilers can accept an options object
    "transpiler": ["ts-node/transpilers/swc-experimental", {
      "jsc": {
        "keepClassNames": true
      }
    }]
  }
}

@cspotcode
Copy link
Collaborator

In the swc docs I see this:

    // Requires v1.2.50 or upper and requires target to be es2016 or upper.
    "keepClassNames": false

But that is no problem; we can automatically set it to false if target is too low.

@cspotcode cspotcode added this to the next milestone May 25, 2021
@ldiqual
Copy link
Author

ldiqual commented May 25, 2021

I can't think of a case where one would not want to keep class names – other than maybe performance impact at the swc-transpilation level if they're doing extra processing. Solution 1. feels like a good path forward and I like the idea of setting the flag automatically depending on target.

@cspotcode
Copy link
Collaborator

other than maybe performance impact at the swc-transpilation level if they're doing extra processing.

Ok, I don't think anyone will notice a couple extra processor cycles in rust.

Thank you for the quick reply. I created a fix in #1344. I need to add tests before I can merge and publish to npm, but in the meantime, I included installation instructions on the pull request so you can test it today.

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

Successfully merging a pull request may close this issue.

2 participants