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

Support for ECMAScript module #1310

Closed
mdantonio opened this issue Oct 29, 2020 · 9 comments
Closed

Support for ECMAScript module #1310

mdantonio opened this issue Oct 29, 2020 · 9 comments

Comments

@mdantonio
Copy link

What version of Ajv you are you using?

6.12.5

What problem do you want to solve?

Hello, I'm opening this issue because I did not find other discussions about that, please redirect me on the proper issue if you are already discussing it. I was looking for any solution for the Angular 10 warnings (CommonJS or AMD dependencies can cause optimization bailouts.), rather that simply ignore them by configuring allowedCommonJsDependencies in angular.json.

What do you think is the correct solution to problem?

I realized that a ECMAScript module would be needed to make Angular happy. Is there any plan to build ajv in that way?

Will you be able to implement it?

Unfortunately not

@epoberezkin
Copy link
Member

Does v7 solve it?

@mdantonio
Copy link
Author

I just tried with 7.0.0-beta.3 and the warning is still there

WARNING in validate.ts depends on 'ajv'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

Probably the new version changed the interface, now I got errors:

ERROR in validate.ts:5:13 - error TS2349: This expression is not callable.
  Type 'typeof import("/app/node_modules/ajv/dist/ajv")' has no call signatures.

5 const ajv = Ajv({

I changed:

import * as Ajv from "ajv";

const ajv = Ajv({
  allErrors: true,
  strictDefaults: true,
  strictKeywords: true,
  strictNumbers: true,
});

into:

import Ajv from "ajv";

const ajv = new Ajv({
  allErrors: true,
});

(strictDefaults, strictKeywords, strictNumbers were no longer recognized)

With this fix the error at compile time disappeared, but then I have troubles at runtime. Please note that I'm trying a fast upgrade from v6 to v7 without studying any migration guide or changelogs, just to verify the commonjs dependencies warning. Probably I'm doing something wrong, but I think that it is no so important to make it work now (I'm waiting the final v7 release before starting the upgrade).

Can I test something more? Do you have any suggestion to change the import or code to prevent the warning?

@epoberezkin
Copy link
Member

epoberezkin commented Nov 7, 2020

Type 'typeof import("/app/node_modules/ajv/dist/ajv")' has no call signatures.

Ajv is ES6 class in v7, it is not possible to make it callable without new.

(strictDefaults, strictKeywords, strictNumbers were no longer recognized)

strict mode is now enabled by default, it includes the functionality of all these option and some other restrictions - see docs/strict-mode.md

I have troubles at runtime.

What exactly? It would help to know what fails.

v7 is functionally backwards compatible, excluding draft-04 schemas support, but it has different defaults and formats were split to ajv-formats package. "Fast upgrade" can probably work for trivial cases, but in most cases it would help reviewing the release notes...

Do you have any suggestion to change the import or code to prevent the warning?

import Ajv from "ajv" works from me without any warnings... There may be some changes needed in tsconfig.json?

What is the code that generates the warning?

Ajv is exported in this way, to be backwards compatible:

export default class Ajv { /* ... */ }
module.exports = Ajv
module.exports.default = Ajv

Which lines above cause the warnings? If you remove 2nd and 3rd do you still have it?

Another issue I recently discovered and not yet sure how best to change that these exports overwrite all other exports that are not types.

For example, even though I have these exports in Ajv:

export {_, str, stringify, nil, Name, Code, CodeGen, CodeGenOptions} from "./compile/codegen"

I can import types Name etc., but not other things from this export.

I didn't look yet how best to address it.

@mdantonio
Copy link
Author

Thank you for the help @epoberezkin, I would like to be more helpful in my reporting but...

What exactly? It would help to know what fails.

The problem is that I don't really understand the runtime error.
This is the stack:

Observable.js:53 TypeError: You provided 'null' where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.
    at subscribeTo (subscribeTo.js:27)
    at innerSubscribe (innerSubscribe.js:69)
    at CatchSubscriber.error (catchError.js:36)
    at MapSubscriber._error (Subscriber.js:75)
    at MapSubscriber.error (Subscriber.js:55)
    at CatchSubscriber.notifyError (innerSubscribe.js:45)
    at SimpleInnerSubscriber._error (innerSubscribe.js:13)
    at SimpleInnerSubscriber.error (Subscriber.js:55)
    at Observable._Observable__WEBPACK_IMPORTED_MODULE_0__.Observable.scheduler.schedule.error.error [as _subscribe] (throwError.js:4)
    at Observable._trySubscribe (Observable.js:42)

And the offending line seems to be this:

  const validator = ajv.getSchema("#/definitions/" + ref);

if I stop my validation function just before this line no exception is raised. It works fine with 6.12.6 (I only changed the version in package.json, changed the import and change the object creation by adding the new).

What is the code that generates the warning?

I'm not very helpful again... I tried to remove both the lines from dist/ajv.js (is the correct file?) but the warning is still there

However, I don't want to waste your time with badly reported issues and confused errors. If my reports are helpful to you to understand the issue, we can continue, otherwise I may back to you with better reporting once migrated my app to ajv 7 (but I would like to do this after the official release)

Thank you again for your help!

@epoberezkin
Copy link
Member

The error is unrelated to ajv, it is because ajv.getSchema("#/definitions/" + ref); returns undefined, but it's not clear why - what are Ajv options and which schema is returned by this call in v6?

I may back to you with better reporting once migrated my app to ajv 7 (but I would like to do this after the official release)

Sure - I will keep the issue open for now

@mdantonio
Copy link
Author

Once back to v6 I put a couple of console.log around the ajv.getSchema

 48  console.log(ref);
 49  const validator = ajv.getSchema("#/definitions/" + ref);
 50  console.log(validator);

->

validate.ts:48 User

validate.ts:50 ƒ (data, dataPath, parentData, parentDataProperty, rootData) { 'use strict';  var vErrors = null;  var errors = 0;      if (rootData === undefined) rootData = data;  if ((data && typeof data === "objec…

"#/definitions/User" is added with a ajv.addSchema by getting the json schema from a json file created with ts-json-schema-generator from a typescript interface

json file:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    [...]
    "Group": {
      "additionalProperties": false,
      "properties": {
        "fullname": {
          "type": "string"
        },
        "members": {
          "items": {
            "additionalProperties": false,
            "properties": {
              "email": {
                "format": "email",
                "type": "string"
              },
              "name": {
                "type": "string"
              },
              "surname": {
                "type": "string"
              },
              "uuid": {
                "type": "string"
              }
            },
            "required": [
              "email",
              "name",
              "surname",
              "uuid"
            ],
            "type": "object"
          },
          "type": "array"
        },
        "shortname": {
          "type": "string"
        },
        "uuid": {
          "type": "string"
        }
      },
      "required": [
        "fullname",
        "shortname",
        "uuid"
      ],
      "type": "object"
    },
    [...]
    "User": {
      "additionalProperties": false,
      "properties": {
        "email": {
          "format": "email",
          "type": "string"
        },
        "first_login": {
          "format": "date-time",
          "type": [
            "string",
            "null"
          ]
        },
        "group": {
          "anyOf": [
            {
              "$ref": "#/definitions/Group"
            },
            {
              "type": "null"
            }
          ]
        },
        "isAdmin": {
          "type": "boolean"
        },
        "isCoordinator": {
          "type": "boolean"
        },
        "isStaff": {
          "type": "boolean"
        },
        "is_active": {
          "type": "boolean"
        },
        "last_login": {
          "format": "date-time",
          "type": [
            "string",
            "null"
          ]
        },
        "last_password_change": {
          "format": "date-time",
          "type": [
            "string",
            "null"
          ]
        },
        "name": {
          "type": "string"
        },
        "privacy_accepted": {
          "type": "boolean"
        },
        "roles": {
          "additionalProperties": {
            "type": "string"
          },
          "type": "object"
        },
        "surname": {
          "type": "string"
        },
        "uuid": {
          "type": "string"
        }
      },
      "required": [
        "email",
        "first_login",
        "group",
        "isAdmin",
        "isCoordinator",
        "isStaff",
        "is_active",
        "last_login",
        "last_password_change",
        "name",
        "privacy_accepted",
        "roles",
        "surname",
        "uuid"
      ],
      "type": "object"
    }
  }
}

Options in v6:

const ajv = Ajv({
  allErrors: true,
  strictDefaults: true,
  strictKeywords: true,
  strictNumbers: true,
});

options in v7:

const ajv = new Ajv({
  allErrors: true
});

@epoberezkin
Copy link
Member

Btw, the exports are now targeting ESM modules and typescript - see Getting started. Yet to look at the issue.

@epoberezkin
Copy link
Member

Going to close it. Please review the release notes - there are some changes in v7 that in many cases would make quick upgrade not possible. Please resubmit a specific failing case if it is still there.

@mdantonio
Copy link
Author

Hello @epoberezkin
I'm just testing the upgrade and I confirm the resolution of this issue, thank you for the effort

I fixed (or better, bypassed) the runtime error reported above by enabling the standalone generation of validation function. This works well with v7.0.1 except for the execution of tests with karma that fail due to duplicated validation functions as reported in #1361.
v7.0.2 fixed this problem but it seems that introduces a new problem with ajv-formats, I will open a new issues for that

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

No branches or pull requests

2 participants