Skip to content

Synchronous validation #22

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

Closed
llwt opened this issue Jul 11, 2016 · 14 comments · Fixed by #25
Closed

Synchronous validation #22

llwt opened this issue Jul 11, 2016 · 14 comments · Fixed by #25
Labels
type: question Questions about the usage of the library.

Comments

@llwt
Copy link

llwt commented Jul 11, 2016

Is it possible with v0.4 to reproduce the validateOrThrow functionality that was present in v0.3?

We have been using this lib to validate options passed into the constructors of modules/classes where we need to be synchronous. Unfortunately we can't target ES6 yet to get async/await support due to being pinned to node v4 and it'd be a non-trivial amount of work to rewrite the all of our bootstrapping to be asynchronous.

@pleerock
Copy link
Contributor

pleerock commented Jul 12, 2016

Now validator is async and it is reasonable. We cant implement synchronous validation anymore, because in this case we must ignore all async validations. This is not so logical, and it will lead to big confusion for users.

I think even having es6 with async/await will not help to resolve your issue. Maybe its better if you change design of the things you do, and make validation out of your models?

@pleerock pleerock added the type: question Questions about the usage of the library. label Jul 12, 2016
@llwt
Copy link
Author

llwt commented Jul 12, 2016

Understandable. We'll pin to 0.3 till we have a chance to refactor our validation logic. Thanks for the quick reply.

@llwt llwt closed this as completed Jul 12, 2016
@sanex3339
Copy link

sanex3339 commented Jul 12, 2016

+1 for sync validation, because i want to use it in node package. But with async validation all package and all data which that package will return will become async, and this is very bad for me.

Maybe it is possible to make sync validation method for non-async validations?

@llwt
Copy link
Author

llwt commented Jul 13, 2016

@sanex3339

Maybe it is possible to make sync validation methid for non-async validations?

This is how it was implemented before, which I agree seems like the right way to handle it so you can validate in cases where being asynchronous is not possible.

@llwt llwt reopened this Jul 13, 2016
@pleerock
Copy link
Contributor

before it was implemented a bit different, there were not async validators support. I can make another function that validates and IGNORES async validations, but people will try to use sync validation instead of async validation and it means that some validations will not work for them and it will lead them to confusion. Adding another opinions @zakhenry @amcdnl

@amcdnl
Copy link

amcdnl commented Jul 13, 2016

Whats the use case for sync vs async?

@sanex3339
Copy link

sanex3339 commented Jul 13, 2016

I make package https://github.com/sanex3339/javascript-obfuscator

Input - source code. Output - obfuscated code.

I want to validate options object
https://github.com/sanex3339/javascript-obfuscator/blob/optionsValidation/src/Options.ts

With async validation all code after validation will be async and i must return obfuscated code inside callback, so using of my package becomes much more difficult with callbacks. All code which depended on string with obfuscated code becomes async too.

PS: now i use Joi, but validation with decorators looks amazing, i want it.

@llwt
Copy link
Author

llwt commented Jul 13, 2016

@amcdnl,

We are using a similar approach as @sanex3339 with an options class that is merged into passed in options then synchronously validated in the constructor.

export interface LoggerOptions {
  console?: boolean;
  handleExceptions?: boolean;
  consoleTransportOptions?: ConsoleTransportOptions;
  //...
}

export class LoggerDefaults implements LoggerOptions {
  @IsBoolean({groups: ['base']})
  public console: boolean = true;

  @IsBoolean({groups: ['base']})
  public handleExceptions: boolean = false;

  @ValidateNested({groups: ['console']})
  public consoleTransportOptions: ConsoleTransportOptions = new ConsoleTransportDefaults();
  //...
}

export class Logger extends CISModule<LoggerOptions> {
  constructor(options: LoggerOptions = {}) {
    super(options);
    //...
  }

  protected validate() {
    this.validatorOptions.groups.push('base');

    if (this.options.console) {
      this.validatorOptions.groups.push('console');
    }

    super.validate();
  }
  //...
}

Base Module

export abstract class CISModule<T> {
  protected options: T;

  protected validatorOptions: ValidatorOptions = {
    groups: [],
  };

  constructor(options: any = {}) {
    this.options = this.mergeDefaults(this.getDefaultOptions(), options);
    this.validate();
  }

  protected validate() {
    try {
      validateOrThrow(this.options, this.validatorOptions);
    } catch (e) {
      //...
    }
  }

  protected mergeDefaults(defaults: T, options: any): T {
    return _.merge(defaults, options);
  }

  protected abstract getDefaultOptions(): T;
  //...
}

@pleerock
Copy link
Contributor

okay I'll add validateSync method just for you guys and hope this will not lead users confusion

@sanex3339
Copy link

sanex3339 commented Jul 15, 2016

AWESOME!!!!!!!!!

@llwt
Copy link
Author

llwt commented Jul 16, 2016

@pleerock
Thanks man!

@llwt
Copy link
Author

llwt commented Jul 17, 2016

@pleerock
If you end up needing/wanting help implementing this let me know. I'm open to submitting a pull request since you aren't directly using this functionality.

@pleerock
Copy link
Contributor

@llwt Im going to work on it today, I'll let you know if some additional help needed. Thanks

@sanex3339
Copy link

thank you!

@typestack typestack locked and limited conversation to collaborators Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: question Questions about the usage of the library.
Development

Successfully merging a pull request may close this issue.

4 participants