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

design:type wrong for enum values #1160

Closed
hcesar opened this issue Oct 12, 2020 · 37 comments · Fixed by #1248
Closed

design:type wrong for enum values #1160

hcesar opened this issue Oct 12, 2020 · 37 comments · Fixed by #1248

Comments

@hcesar
Copy link

hcesar commented Oct 12, 2020

Describe the bug
In TSC, when you have an enum type, the base enum type is set as design:type.
SWC is setting the enum object itself.

Input code

enum MyEnum {
  x = "xxx",
  y = "yyy"
}

class Xpto {
  @Decorator()
  value!: MyEnum;
}

function Decorator() {
  return function (...args) {};
}

Config

{
  "module": {
    "type": "commonjs"
  },
  "jsc": {
    "target": "es2018",
    "parser": {
      "syntax": "typescript",
      "decorators": true,
      "dynamicImport": false
    },
    "transform": {
      "legacyDecorator": false,
      "decoratorMetadata": true
    }
  }
}

Expected behavior
A clear and concise description of what you expected to happen.

TSC:

__decorate([
    Decorator(),
    __metadata("design:type", String)
], Xpto.prototype, "value", void 0);

SWC:

Reflect.metadata("design:type", typeof MyEnum === "undefined" ? Object : MyEnum)
@hcesar hcesar added the C-bug label Oct 12, 2020
@kdy1 kdy1 added this to the v1.2.37 milestone Oct 12, 2020
@kdy1 kdy1 removed this from the v1.2.37 milestone Oct 29, 2020
@Exilz
Copy link

Exilz commented Dec 3, 2020

@kdy1 Any news regarding this issue ? We have a similar problem that's preventing our team from switching to swc. We're using https://github.com/RobinBuschmann/sequelize-typescript and decorators are a problem for us right now.

@Column
status: TProcessingStates;

TSC:

__decorate([
    decorators_1.Groups([{ operation: 'read' }, { operation: 'write' }]),
    sequelize_typescript_1.Column,
    __metadata("design:type", String)
], Collage.prototype, "status", void 0);

SWC:

Reflect.metadata("design:type", typeof _mediaManager.TProcessingStates === "undefined" ? Object : _mediaManager.TProcessingStates)

Switching to swc would be a game changer for our team.

@kdy1
Copy link
Member

kdy1 commented Dec 3, 2020

@Exilz I'll fix this soon.

I expect today or tomorrow (It's almost 11 PM here, so...)

@kdy1 kdy1 added this to the v1.2.40 milestone Dec 3, 2020
@kdy1 kdy1 mentioned this issue Dec 3, 2020
1 task
@kdy1 kdy1 closed this as completed in #1248 Dec 3, 2020
kdy1 added a commit that referenced this issue Dec 3, 2020
swc_ecma_transforms:
 - Emit proper typename for `design:type` used with enum. (#1160)
@kdy1
Copy link
Member

kdy1 commented Dec 3, 2020

Fixed it.

May I list your company on the site as swc user after the migration?

@Exilz
Copy link

Exilz commented Dec 3, 2020

@kdy1 I need to ask the CEO permission but I guess we'd be happy to be listed on the website !
How do I go about testing your fix ? Do I need to build the source myself ? I've never used rust before so I'm not sure where to start.

EDIT: nevermind, I just realized you have a CI that's compiling it right now.

@kdy1
Copy link
Member

kdy1 commented Dec 3, 2020

@Exilz I'm now in progress of publishing. Github actions will publish new version.
Typically it takes about one hour.

Oh. I noticed that github updates comments real-time.

@Exilz
Copy link

Exilz commented Dec 3, 2020

@kdy1 thanks a lot for the update. I'm still in the process of refactoring our code, but it looks like we're one step closer to migrating to swc.

However, I noticed a couple of things :

  • Using enums as the issue stated, it doesn't work if the enum is imported from another file. It compiles properly when the enum is declared in the same file, though.
import { TPublicationStatus } from '../publicationStatus'; 

class MyClass {

  @Column
   publicationStatus: TPublicationStatus;
}

which compiles to

_dec31 = typeof Reflect !== "undefined" && typeof Reflect.metadata === "function" && Reflect.metadata("design:type", typeof _publicationStatus.TPublicationStatus === "undefined" ? Object : _publicationStatus.TPublicationStatus)
  • A kind of related issue happens using types instead of enums, as in the following exemple :
    @Column
    type: 'reps' | 'time';

which compiles to

 _dec24 = typeof Reflect !== "undefined" && typeof Reflect.metadata === "function" && Reflect.metadata("design:type", void 0)

This is also true when declaring the type separately and not directly in the class.

Should I open a new issue ? Or two ?

@kdy1
Copy link
Member

kdy1 commented Dec 3, 2020

I'll just reopen this.
But I'm not sure if knowing a type is enum without evaluating dependencies.
(tsc works by loading all file, while swc and babel work in one-by-one manner)

Evaluating deps is not hard, but I'm not sure about the api change.

@kdy1 kdy1 reopened this Dec 3, 2020
@Exilz
Copy link

Exilz commented Dec 3, 2020

@kdy1 I understand what you mean, this is quite a big change for the project as a whole.
However, I guess importing a type or an enum and using it like this is a common usage, right ?

If this isn't implemented, you'd need to document it and make sure people are aware of this because it's quite "sneaky", and the errors it can generate when running your app won't make sense to most people.

Maybe that's something the maintainers need to discuss further.

@kdy1
Copy link
Member

kdy1 commented Dec 3, 2020

I don't think it's sneaky. Why it's sneaky?
It is just misunderstanding about the way tools work.

@Exilz
Copy link

Exilz commented Dec 3, 2020

Well, because something that's working within a single file will break when using imports.
Sneaky isn't quite the right word, but it will probably confuse people.

@hcesar
Copy link
Author

hcesar commented Dec 4, 2020

I believe this single file limitation will be a deal breaker for most people interested in SWC.

If SWC can't be a transparent replacement for TSC, there will be just very few use cases where it will be used.

@kdy1
Copy link
Member

kdy1 commented Dec 4, 2020

I'm actively working on the type system, which will allow any transforms requiring type informations.

Even though the issue can be fixed by simple hack, I'm not sure if it's correct way to go. It requires lots of opinionated approach.

@sunnylqm
Copy link

Would appreciate a simple hack first (or as an option) since type system is a long way to go.

@Exilz
Copy link

Exilz commented Feb 22, 2021

I agree with @sunnylqm, we've been eager to hop on the swc train ever since we've discovered it, but this issue is still a show stopper for us.
@kdy1 Do you have any rough timeline regarding the availability of the type system ? Or maybe the aforementioned hack ?

@kdy1
Copy link
Member

kdy1 commented Feb 22, 2021

@Exilz No. I don't have one.

@kdy1 kdy1 modified the milestones: v1.2.40, v1.2.51 Mar 4, 2021
@kdy1
Copy link
Member

kdy1 commented Mar 4, 2021

I have questions regarding the issue.

For the context, I want to implement this for the next version but this is fairly complex, mainly due to circular imports.
Also, as it is also used by deno, there are some restrictions in the APIs.

To support it correctly, loader abstraction and a caching layer are required.

So my first question is

  • Are you guys using spack?

It already have loader abstraction and a caching layer, so it would be easier.

Also, I'm not sure about the api.
The api of swc is designed after babel, which handles one file at a time.
But tsc compiles all dependencies even if the user only passes one file.

I think flags like --deps (and jsc.includeDeps: true in .swcrc) would work.

  • Does this API seem reasonable?

If not, please feel free to tell me.

@Exilz
Copy link

Exilz commented Mar 4, 2021

@kdy1 We're not using spack. Our setup is fairly simple so using only the CLI is enough for us.
I guess it would be acceptable for you to require people to use your bundler in order to achieve such things as long as it's clearly stated in the documentation.

The deps flag / includeDeps option would be awesome. I'm just wondering if it should be enabled by default.

@sunnylqm
Copy link

sunnylqm commented Mar 4, 2021

Is includeDeps equivalent to isolatedModules?

@kdy1
Copy link
Member

kdy1 commented Mar 4, 2021

@sunnylqm No. The purpose of the option is to prevent breaking things.
e.g. npx swc src/ should work identically.
But with includeDeps: true, the invocation can be changed to npx swc src/index.ts.

@sunnylqm
Copy link

sunnylqm commented Mar 4, 2021

Compiling one by one is always a compromise for babel. If swc aims to work the same way as tsc then why not. Shouldn't swc eventually become a full feature drop in replacement?

@sunnylqm
Copy link

We can try some alpha versions to see if it can solve some problems

@kdubrovnyi
Copy link

@kdy1 hello and thanks for the awesome product!
It works amazing, but we bumped into the same issue with decorator metadata, while trying to adopt SWC in the company.
I see that it's been moved into Blocked by type checker milestone.
Is the type-checking feature has been planned to be implemented in any foreseeable future?
Thanks.

@kdy1
Copy link
Member

kdy1 commented May 7, 2022

I think this can be fixed without the type checker.
The main problem is internal apis, though.

@kdy1
Copy link
Member

kdy1 commented May 7, 2022

Does this work for tsc with isolatedmodules?

@kdubrovnyi
Copy link

@kdy1 thanks for the reply.

Just checked, the project has no errors when running tsc with --isolatedModules switch.
With swc though, the decorator metadata for imported enum has been generated in a way that won't work with TypeOrm - throws an error in runtime (the same as specified in the issue).

Of course, if this can be fixed, it will help a great deal to move our project to use swc.
Thanks!

@mfcodeworks
Copy link

Encountering the same issue with sequelize-typescript, everything else is working perfectly but without Sequelize we can't run the app or @swc-node/jest tests

@drFabio
Copy link

drFabio commented Jun 1, 2022

Encountering the same issue with sequelize-typescript, everything else is working perfectly but without Sequelize we can't run the app or @swc-node/jest tests

See if this typeorm sidestep I made is somehow portable to sequelize. Essentially there were infered enum types that did not work and needed to be explicitly set

@mfcodeworks
Copy link

Encountering the same issue with sequelize-typescript, everything else is working perfectly but without Sequelize we can't run the app or @swc-node/jest tests

See if this typeorm sidestep I made is somehow portable to sequelize. Essentially there were infered enum types that did not work and needed to be explicitly set

Thanks, it did actually prevent one error from popping up, I'll see if applying this to all enum types solves the issue

@mfcodeworks
Copy link

Encountering the same issue with sequelize-typescript, everything else is working perfectly but without Sequelize we can't run the app or @swc-node/jest tests

See if this typeorm sidestep I made is somehow portable to sequelize. Essentially there were infered enum types that did not work and needed to be explicitly set

Amazing, this solved all the issues. Somehow tsc infers correct types when building but swc didn't, but by giving explicit types to Sequelize it works without issue. Thank you!

@dsteinbach-ep
Copy link

@drFabio

Encountering the same issue with sequelize-typescript, everything else is working perfectly but without Sequelize we can't run the app or @swc-node/jest tests

See if this typeorm sidestep I made is somehow portable to sequelize. Essentially there were infered enum types that did not work and needed to be explicitly set

What solution are you referring to? I do not see anything in this thread. I have the same issues with TypeOrm.

@swc-project swc-project locked as off-topic and limited conversation to collaborators Jun 15, 2022
@swc-project swc-project unlocked this conversation Jun 15, 2022
@mfcodeworks
Copy link

@drFabio

Encountering the same issue with sequelize-typescript, everything else is working perfectly but without Sequelize we can't run the app or @swc-node/jest tests

See if this typeorm sidestep I made is somehow portable to sequelize. Essentially there were infered enum types that did not work and needed to be explicitly set

What solution are you referring to? I do not see anything in this thread. I have the same issues with TypeOrm.

For me it was to explicitly set the types, it seems like there's a problem inferring the type when built with swc, but if I explicitly set them i.e. decorator @Column({ type: DataTypes.STRING }), then it works without issue.

I'm not sure if TypeORM uses a different format but setting the type explicitly in the decorator solved any Sequelize issues.

@stefanli
Copy link

Just wanna chime in that I ran into what I think is the same issue. I have a multi-project setup where projects A, and B references enums in project C (a shared library) through tsconfig paths. The enums work fine when using ts-loader (webpack) but when switching to SWC and swc-loader the enums are undefined at runtime. This is a bit of a bummer since the speed gains I got from SWC were amazing, but had to switch back to ts-loader for now.

Thank you for all the hard work on this! Looking forward to switching over when this is fixed.

@omerd-cyera
Copy link

+1
I'm using typeorm too, and trying to migration over to swc. Did anyone find a workaround for this?

@kdy1
Copy link
Member

kdy1 commented Jan 23, 2024

Closing as we are not going to implement project mode of tsc

@kdy1 kdy1 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 23, 2024
@swc-bot
Copy link
Collaborator

swc-bot commented Feb 22, 2024

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.