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

feat: support class properties transformation #361

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

fgcoelho
Copy link

Resolves

This PR resolves Originally posted by @cshomo11 in #57

MWE

MWE (minimal reproduction) repository

For testing the MWE, just hit pnpm install and then pnpm working or pnpm not-working.

Description

We should not be using the babel-plugin-syntax-class-properties:

image

Just as written in the babel docs, the plugin does not bring transform, so it causes an issue with class-transfomer (and probably other packages):

[jiti] [init] version: 2.4.2 module-cache: true fs-cache: false interop-defaults: true
[jiti] [transpile] [esm] ./src/class-validator/env.config.ts (51.721ms)
[jiti] [native] [import] workspace/my-lib/packages/my-lib/dist/core/core.cjs
[jiti] [native] [import] workspace/my-lib/node_modules/.pnpm/[email protected]/node_modules/dotenv/lib/main.js
[jiti] [native] [import] workspace/my-lib/node_modules/.pnpm/[email protected]/node_modules/class-validator/cjs/index.js
[jiti] [native] [import] workspace/my-lib/node_modules/.pnpm/[email protected]/node_modules/class-transformer/cjs/index.js
workspace/my-lib/packages/playground/src/class-validator/env.config.ts:7
<TRANSPILED CODE THAT FAILED GOES HERE>

Error: Decorating class property failed. Please ensure that transform-class-properties is enabled and runs after the decorators transform.
    at _initializerWarningHelper (workspace/my-lib/packages/playground/src/class-validator/env.config.ts:7:767)
    at <instance_members_initializer> (workspace/my-lib/packages/playground/src/class-validator/env.config.ts:13:141)
    at new Schema (workspace/my-lib/packages/playground/src/class-validator/env.config.ts:13:116)
    at TransformOperationExecutor.transform (workspace/my-lib/node_modules/.pnpm/[email protected]/node_modules/class-transformer/cjs/TransformOperationExecutor.js:147:32)
    at ClassTransformer.plainToInstance (workspace/my-lib/node_modules/.pnpm/[email protected]/node_modules/class-transformer/cjs/ClassTransformer.js:27:25)
    at plainToInstance (workspace/my-lib/node_modules/.pnpm/[email protected]/node_modules/class-transformer/cjs/index.js:38:29)
    at validate (workspace/my-lib/packages/playground/src/class-validator/env.config.ts:27:60)
    at Command.<anonymous> (workspace/my-lib/packages/my-lib/dist/cli/cli.cjs:334:39)

Node.js v22.3.0
 ELIFECYCLE  Command failed with exit code 1.

The only fix without merging is including the plugin by yourself:

import pluginTransformClassProperties from "@babel/plugin-transform-class-properties";

const jiti = createJiti(import.meta.url, {
   transformOptions: {
     ts: true,
     babel: {
       plugins: [pluginTransformClassProperties],
     },
   },
 });

Transpiled code that failed

var _classTransformer = await jitiImport("class-transformer");
var _dec, _dec2, _dec3, _dec4, _class, _descriptor, _descriptor2;
function _interopRequireDefault(e) {
    return e && e.__esModule ? e : { default: e };
}
function _applyDecoratedDescriptor(i, e, r, n, l) {
    var a = {};
    return (
        Object.keys(n).forEach(function (i) {
            a[i] = n[i];
        }),
        (a.enumerable = !!a.enumerable),
        (a.configurable = !!a.configurable),
        ("value" in a || a.initializer) && (a.writable = !0),
        (a = r
            .slice()
            .reverse()
            .reduce(function (r, n) {
                return n(i, e, r) || r;
            }, a)),
        l && void 0 !== a.initializer && ((a.value = a.initializer ? a.initializer.call(l) : void 0), (a.initializer = void 0)),
        void 0 === a.initializer ? (Object.defineProperty(i, e, a), null) : a
    );
}
function _initializerWarningHelper(r, e) {
    throw Error("Decorating class property failed. Please ensure that transform-class-properties is enabled and runs after the decorators transform.");
}
let;

@fgcoelho fgcoelho changed the title fix: wrong babel plugin causes transform-class-properties error fix: change babel plugin to support class properties transformations Mar 4, 2025
@fgcoelho fgcoelho force-pushed the fix-transform-class-properties-error branch from e2650af to 17d7f43 Compare March 4, 2025 23:18
@fgcoelho fgcoelho changed the title fix: change babel plugin to support class properties transformations fix: change babel plugin to support class properties transformation Mar 4, 2025
@pi0 pi0 changed the title fix: change babel plugin to support class properties transformation feat: support class properties transformation Mar 7, 2025
@pi0
Copy link
Member

pi0 commented Mar 7, 2025

Thank you for the PR and sorry for late review. I have added a dep reproduction and it fixes issue.

Testing on some projects, I found effect of this change is that babel is transpiling more than it should (for example private

export class User {
  #id = 0;

  constructor() {
    this.#init();
    console.log(this.#id);
  }

  #init() {
    this.#id = Math.random();
  }
}

Was working before without any transform (JS has native private field/method support) but now fails:

[Error: TRANSFORM_ERROR: Class private methods are not enabled. Please add @babel/plugin-transform-private-methods to your configuration.

Of course we can add more transforms but it is raising a question for me if it is safe thing to do.

One way we can do to move this faster is to make transform opt-in on top of syntax.

@octet-stream
Copy link

One way we can do to move this faster is to make transform opt-in on top of syntax.

Fine by me, but there might be more problems, so maybe things should be tested.

@fgcoelho
Copy link
Author

fgcoelho commented Mar 7, 2025

plugin-transform-class-properties only deals with classes, so every other transform we may need will probably be class-related. Therefore, I don't think we will need many more transforms.

Still, I support the idea of proceeding with opt-in transforms for now so we can address any arising issues and, after some time, merge them as enabled by default 👍

@fgcoelho
Copy link
Author

Hey guys 👋

I've implemented the "experimentalTransforms" option, along with a fixture for typestack packages.

You can test what happens without the option by going into fixtures.test.ts and setting JITI_EXPERIMENTAL_TRANSFORMS: "false".

I've also fixed the missing MikroOrm fixture import in deps/index.ts

@pi0
Copy link
Member

pi0 commented Mar 10, 2025

Thank you! Perhaps better call it more explicitly transformClassProps ?

@fgcoelho
Copy link
Author

I would advocate for the experimental keyword if we were to make them default someday. We may need to add other transforms beyond the current two, so who knows if all experimental transforms will be related to transforming class properties?

Also, I don't think people should enable this option unless they got a real reason for it, like these edge cases with typestack and mikro-orm.

I may be wrong, let me know 🧐

@pi0
Copy link
Member

pi0 commented Mar 10, 2025

I think we would never enable this by default, as it implies more transformation and less proximity to runtime native behavior .

@octet-stream
Copy link

octet-stream commented Mar 10, 2025

I think we would never enable this by default, as it implies more transformation and less proximity to runtime native behavior.

Does that mean that jiti's not aiming to be compatible with tsc as much as possible? Because this specific feature should improve said compatibility.
I think if not, maybe it should be opt-in under more generic flag, of course if there's more things to improve there, aside from class transformations.

@pi0
Copy link
Member

pi0 commented Mar 10, 2025

Yes generic opt-in flag is cool 👍🏼 (my goal for jiti is to be as close to native Node.js as possible by default.)

@octet-stream
Copy link

my goal for jiti is to be as close to native Node.js as possible by default

So, erasableSyntaxOnly + rewriteRelativeImportExtensions + allowImportingTsExtensions compatibility by default, got it. Maybe it needs to be mentioned in readme somewhere? Because I don't really see it, but maybe it's just me :D

@pi0
Copy link
Member

pi0 commented Mar 10, 2025

Jiti supports all of those (TS features). I meant runtime (JS) syntax support like class props. It can be easily an opt in feature for when needed for class props :)

@fgcoelho
Copy link
Author

@pi0 Done. If it ain't going to be default anywhere in the future, then transformClassProps seems right.

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 this pull request may close these issues.

Typescript: Add support for class properties and emitDecoratorMetadata
3 participants