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

feature: allow awaiting Promises on properties #549

Open
NoNameProvided opened this issue Jan 14, 2021 · 9 comments · May be fixed by #1490
Open

feature: allow awaiting Promises on properties #549

NoNameProvided opened this issue Jan 14, 2021 · 9 comments · May be fixed by #1490
Assignees
Labels
flag: needs discussion Issues which needs discussion before implementation. type: feature Issues related to new features.

Comments

@NoNameProvided
Copy link
Member

NoNameProvided commented Jan 14, 2021

Description

There had been multiple requests about allowing awaiting promises during transformation. This issue is the tracking issue for the feature.

Proposed solution

Our API is currently sync and doesn't support async operations. The main requirements for the feature should be:

  • adding async support should not decrease the performance of sync only validation
  • users using sync only validation should not deal with async operations (so no unified Promise only response)
  • some form of timeout should be supported for async validation

We have multiple options to implement this:

  • we can introduce an option to the validation methods which signals an async validation, in that case a Promise should be returned
  • we can duplicate the API surface and create a fnAsync method for all validation method

Questions

How to handle promises in sync validation?
Generally, I think Promises in sync mode should be converted to undefined. However, we may want to add a transformation option that forbids promises in sync validation. This can be useful for users who use both methods.

to be continued... this issue will be expanded with more implementation details before work starts

Please do not comment non-related stuff like +1 or waiting for this. If you want to express interest you can like the issue.

@NoNameProvided NoNameProvided added type: feature Issues related to new features. flag: needs discussion Issues which needs discussion before implementation. labels Jan 14, 2021
@NoNameProvided NoNameProvided self-assigned this Jan 14, 2021
@NoNameProvided
Copy link
Member Author

A related fix has been merged in #463, which prevents a JS error from happening when a Promise is found on a property during transformation.

@NoNameProvided
Copy link
Member Author

A related fix has been merged in #262.

@cloudratha

This comment has been minimized.

@XavierJece
Copy link

  class User {
    @Transform(async ({ value }) => generateAvatarUrl(value))
    avatar?: string;
  }

I have this User class that has an avatar attribute that calls an asynchronous function to generate the temporary storage url. The return after using classToPlan is:

{
   avatar: {}
}

From what I analyzed the avatar attribute is still being a promise, I'm not able to solve someone help me? I'm using version "class-transformer": "^0.4.0".

@flisboac
Copy link

I think we have three distinct requirements, that are all blocked by the fact that class-transformer does not support promises:

  1. Make class-transformer await on promises that may come from the input object's properties.
  2. Integrate with DI containers that have promise-based APIs.
  3. Allow for custom transformations to return promises, and make class-transformer await on them before assigning to properties.

For (3), the same solution applied to (1) may be applied here. If we are to reuse the sync part of the library (and depending on the order in which we apply the promise-based part), the promises in the input object must be ignored, and be passed as-is. I'm not sure how the library deals with promises today; if it blocks promises, or raises errors when it sees one, that behaviour may perhaps be deactivated by a new flag, like async: true, that we will need to include in each front-facing function of the API. An advantage of this approach is that we can include new signatures for those functions (returning Promises) based on the presence of this flag.

To indicate which DI container to use, we could repeat a mechanism similar to the one implemented by class-validator.

But still, I think that some improvements could be made to the API surface to support for DI at the decoration level. It doesn't need to go into a high level of specificity. All DI containers support retrieving dependencies by token, so that could be the minimum support we could add, as a new decorator. For example:

export interface InjectedTransformOptions {
  token: string | symbol | () => (string | symbol | Constructor<T> | AbstractClass<T>);
}

export function InjectedTransform(options: InjectedTransformOptions): PropertyDecorator {
  // ...
}

Then, before returning the transformed object to the user, we can check for the async flag, and if activated, we resolve all promises before returning. I don't think it'll impact performance of the sync-side of things that much, as long as we deal with promises explicitly via branching (i.e. if), instead of making current functions async.

One disadvantage of this approach, though, is the fact that there'll be no way to order how or when these promises will be resolved.


For those that, like me, cannot wait for promise support to land in class-transformer, I made some proof-of-concepts for NestJS DI support here. I haven't tested them yet, though; I'm sharing just to have some early feedback:

Advantage of the sync version is that it does not introduce much, and uses the very same sync API class-transformer presents to us today. Disadvantage is that you'll need to await on properties yourself, in an explicit step, after calling plainToClass, etc.

Advantage of the async version is that it does all the promise resolution for you, with or without a dependency injection (there're two different types of async transformers: async function and injected object). Dependency injection also supports scoped providers. Disadvantage is the introduction of new metadata, and new transform functions (starting with async).

@denbite
Copy link

denbite commented Sep 19, 2021

const plainWithoutPromises = Object.assign(
    {}, 
    yourEntity, 
    {
        promiseProp: await yourEntity.propWithPromise,
        promiseProp2: await yourEntity.propWithPromise2,
        ...
    }
);

plainToClass(YourClass, plainWithoutPromises);

This issue still exists on 19 September, so I've solved it in a such way, maybe it will help someone. The main idea is to give objects without promises to the class transformer.

@SergioSuarezDev
Copy link

I was having issues with GraphQLUpload and InputType

jaydenseric/graphql-upload#276

@mostafa8026
Copy link

In which version, promises work? I have issue using promises in Transform.

@iteratelance
Copy link

Seems there will not be any progress on this. What libraries are others using for async serialization? I've looked at tools like https://www.npmjs.com/package/json-object-mapper and https://www.npmjs.com/package/json-api-serializer but neither seem to support async methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: needs discussion Issues which needs discussion before implementation. type: feature Issues related to new features.
8 participants