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

Validator accepting plugins #552

Merged
merged 4 commits into from
Aug 29, 2020

Conversation

leog
Copy link
Contributor

@leog leog commented Aug 15, 2020

(Thanks for sending a pull request! Please make sure you click the link above to view the contribution guidelines, then fill out the blanks below.)

What does this implement/fix? Explain your changes.

Implements the ability to apply passed plugins to validator

Does this close any currently open issues?

Closes #550

Any relevant logs, error output, etc?

N/A

Any other comments?

N/A

Where has this been tested?

Node.js Versions: 12.16.1

Middy Versions: 1.2.0

AWS SDK Versions: 2.656.0

Todo list

[x] Feature/Fix fully implemented
[x] Added tests
[x] Updated relevant documentation
[x] Updated relevant examples

willfarrell
willfarrell previously approved these changes Aug 16, 2020
@willfarrell
Copy link
Member

Interesting approach, I was thinking something different. I like this better, cleaner.

Thinking out loud here: Maybe avjKeywords should be refactored into this pattern (have it as a default until v2)? Both errors and keywords can take options themselves. Seeing how ajv-errors is really the only plugin missing, what if we included it by default and let the user pass in an object of the plugins w/ options they want to use. Since we don't want to introduce a breaking change it would have to default to {"keywords":{}}.

What do you think?

@leog
Copy link
Contributor Author

leog commented Aug 16, 2020

Thanks for the feedback. And yes, makes sense.

So to task it out if I understood correctly:

  • Let the user pass plugins using the pattern {"keywords":{}} enabling plugins options to be passed as well
  • Use plugins implementation to include out-of-the-box ajv-keywords and ajv-errors

In case the user prefers to apply one of the out-of-the-box plugins for further customization I guess we should prioritize that, which we could as we would be defaulting the plugins similar to options 👍

Applying plugin system internally
@leog
Copy link
Contributor Author

leog commented Aug 16, 2020

Well @willfarrell, I think we might need to leave ajv-errors out after all, as it requires a possible breaking change due to jsonPointers ajv option having to be true, otherwise the plugin sets it automatically giving a warning. Thoughts?

BTW, got i18n into the new plugins system as well, PTAL.

@leog
Copy link
Contributor Author

leog commented Aug 25, 2020

Please let me know if this is OK @willfarrell.

@willfarrell willfarrell merged commit abd9e1f into middyjs:master Aug 29, 2020
@renanwilliam
Copy link

This change broke a running project. I'm receiving this error:

  • Error: Cannot find module 'ajv-keywords'

Is necessary any change or manual library import?

@willfarrell
Copy link
Member

ajv-keywords is included as a dep in the submodule. I'll do some additional testing today.

@leog
Copy link
Contributor Author

leog commented Aug 31, 2020

Let me know if I can help @willfarrell. It may be related to the dynamic require, can't see why at the moment.

@willfarrell
Copy link
Member

@renanwilliam I was able to reproduce your code issue. Are you using serverless with serverless-bundle? ajv-keywords and ajv-i18n were moved to be dynamically required, which seems to be the cause. Debugging and testing out solutions now.

@renanwilliam
Copy link

@willfarrell just additional information, I have added manually as a dependency but keeps throwing the error. Rolling back to ~1.2.0 solves the problem.

@renanwilliam
Copy link

renanwilliam commented Aug 31, 2020

@renanwilliam I was able to reproduce your code issue. Are you using serverless with serverless-bundle? ajv-keywords and ajv-i18n were moved to be dynamically required, which seems to be the cause. Debugging and testing out solutions now.

Yes, I'm using with serverless with serverless-bundle

@willfarrell
Copy link
Member

Did some testing, and was unsuccessful in finding a solution. Updating the bundle config should have done it (see below).

serverless.yml

custom:
  bundle:
    sourcemaps: false
    caching: false
    linting: false
    forceInclude:
      - pg
      - ajv-keywords
      - ajv-i18n
      - ajv-errors

I'm not a webpack expert, so can't take it any furthur at this time. @renanwilliam if you can open an issue at https://github.com/AnomalyInnovations/serverless-bundle/issues/new, I'd be interested in what the solution is and how we can update our docs to prevent this issue for others. cc @AnomalyInnovations

What I can do is update the module to allow the ability to remove all default plugins by passing in ajvOptions:{}. Keep a look out for v1.3.1.

@leog
Copy link
Contributor Author

leog commented Sep 1, 2020

@willfarrell if you could share a reproducible project to test solutions I can give it a try, have a couple of ideas. Thanks @renanwilliam for reporting this and being patient.

@willfarrell
Copy link
Member

I was testing on a private repo. Any serverless project w/ serverless-bundle should invoke the issue.

@ayushsharma82
Copy link

A 'plugin' array would have been more useful considering ajv-keywords got removed in v2 release.

Any way to add the ajv-keywords plugin back in v2?

@willfarrell
Copy link
Member

@ayushsharma82 Please see my response on #698

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Ability to apply plugins to middy/validator
4 participants