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

Broken codegen of class with decorated method when using jsc.module.lazy #1135

Closed
paul-soporan opened this issue Oct 4, 2020 · 3 comments · Fixed by #4758
Closed

Broken codegen of class with decorated method when using jsc.module.lazy #1135

paul-soporan opened this issue Oct 4, 2020 · 3 comments · Fixed by #4758
Labels
Milestone

Comments

@paul-soporan
Copy link

paul-soporan commented Oct 4, 2020

Describe the bug

Edit: I've used module as an example of an external module below, but I've now realized that it might be confused with the module Node builtin. The issue exists no matter what name the external module has.

Codegen breaks when all of the conditions below are met:

  • jsc.module.type has to be set to commonjs.
  • jsc.module.lazy has to be set to true.
  • A base class has to be imported from an external module.
  • Another class has to extend the base class.
  • The subclass has to have a decorated method, decorated with a decorator that is a static property on the base class.

Input code

import { Class } from 'module';

class MyClass extends Class {
  @Class.Decorator()
  async method() {}
}

Config

{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "decorators": true
    },
    "target": "es2020",
    "transform": {
      "legacyDecorator": true
    }
  },
  "module": {
    "type": "commonjs",
    "lazy": true
  }
}

Emitted code

"use strict";
var _module = require("module");
function _applyDecoratedDescriptor(target, property, decorators, descriptor, context) {
    var desc = {
    };
    Object.keys(descriptor).forEach(function(key) {
        desc[key] = descriptor[key];
    });
    desc.enumerable = !!desc.enumerable;
    desc.configurable = !!desc.configurable;
    if ("value" in desc || desc.initializer) {
        desc.writable = true;
    }
    desc = decorators.slice().reverse().reduce(function(desc, decorator) {
        return decorator(target, property, desc) || desc;
    }, desc);
    if (context && desc.initializer !== void 0) {
        desc.value = desc.initializer ? desc.initializer.call(context) : void 0;
        desc.initializer = undefined;
    }
    if (desc.initializer === void 0) {
        Object.defineProperty(target, property, desc);
        desc = null;
    }
    return desc;
}
var _class, _dec;
let MyClass = ((_class = function() {
    class MyClass extends _module().Class {
        async method() {
        }
    }
    return MyClass;
}()) || _class, _dec = _module.Class.Decorator(), _applyDecoratedDescriptor(_class.prototype, "method", [
    _dec
], Object.getOwnPropertyDescriptor(_class.prototype, "method"), _class.prototype), _class);

Expected behavior

The emitted code includes class MyClass extends _module().Class. Note the _module() call, which shouldn't happen, as _module is assigned the required module which isn't callable. This leads to a runtime error (TypeError: _module is not a function).

Version
The version of @swc/core: 1.2.35

Additional context

This issue doesn't exist when jsc.module.lazy is set to false.

@kdy1 kdy1 added this to the v1.2.36 milestone Oct 5, 2020
@kdy1 kdy1 mentioned this issue Oct 6, 2020
3 tasks
@kdy1 kdy1 assigned kdy1 and unassigned kdy1 Oct 6, 2020
@kdy1 kdy1 removed this from the v1.2.36 milestone Oct 6, 2020
@kdy1
Copy link
Member

kdy1 commented Oct 6, 2020

As you used Class unconditionally, I think it should be

class MyClass extends _module.Class {
    async method() {
    }
}

@kdy1 kdy1 self-assigned this Oct 6, 2020
@kdy1 kdy1 added this to the v1.2.36 milestone Oct 6, 2020
@paul-soporan
Copy link
Author

Yep, that's how it should be. It's correctly emitted as _module.Class when jsc.module.lazy is set to false (the default), but it becomes _module().Class when jsc.module.lazy is set to true.

That also happens to be the only difference between the outputs (diff lazy.js non-lazy.js):

29c29
<     class MyClass extends _module().Class {
---
>     class MyClass extends _module.Class {

@kdy1 kdy1 modified the milestones: v1.2.36, v1.2.37 Oct 6, 2020
@kdy1 kdy1 removed their assignment Oct 6, 2020
@kdy1 kdy1 removed this from the v1.2.37 milestone Oct 29, 2020
@kdy1 kdy1 added this to the Planned milestone Mar 12, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.2.199 Jun 11, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.2.206 Jun 27, 2022
@swc-bot
Copy link
Collaborator

swc-bot commented Oct 16, 2022

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 Oct 16, 2022
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.

3 participants