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: compilers factory (transient ajv dependency) #2862

Merged
merged 21 commits into from
May 21, 2021

Conversation

Eomm
Copy link
Member

@Eomm Eomm commented Feb 19, 2021

This PR is the follow up of #2755
and closes #2742
and include a new fastify repo: fastify/ajv-compiler#1

Introducing the ajv-compilers dependency and documenting the compilersFactory option let the user change the ajv@x version that fastify will use.

By default, it will be ajv@6 to avoid breaking changes in fastify@3 and we can dispose of an ajv-compilers version that includes ajv@7 to be ready for any issue.

TODO:

  • define a common interface per compilersFactory.buildValidator (factory function that returns a factory) and compilersFactory.buildSerializer (factory function):

const option = Object.assign({
bucket: buildSchemas,
compilersFactory: {
buildValidator: ValidatorSelector(),
buildSerializer: buildDefaultSerializer
}
}, opts)

  • check that the interface support ajv@7 as well - so the user will be able to set this option in fastify@3:
  const fastify = Fastify({
    schemaController: {
      compilersFactory: {
        buildValidator: require('ajv-compiler') // includes ajv@7
      }
    }
  })
  • write docs per compilersFactory

Checklist

cc @inigo-garcia I more than happy if you could help me to push this PR

@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Feb 19, 2021
@Eomm
Copy link
Member Author

Eomm commented Mar 6, 2021

Just an update:

  • add a check that this feature works with ajv-7 (see test)
  • the feature is quite ready, need to solve the first bullet point here

Feedback is appreciated 👍

EDIT 2:

  • missing test

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.


const buildValidatorAJV7 = require('@fastify/ajv-compiler-7')

test('Ajv 7 usage instead of the bundle one', t => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Base automatically changed from master to main March 26, 2021 12:02
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that probably would be great to have a more descriptive changelog for this change. What do you think?

@devlzcode
Copy link

Any updates on this PR?

@Eomm
Copy link
Member Author

Eomm commented Apr 22, 2021

Any updates on this PR?

I must write docs per serializerOpts and a better changelog

Moreover, I have some trouble with ci - that is working on my laptop 😕

@Eomm
Copy link
Member Author

Eomm commented Apr 22, 2021

It fails on node 16!.. checking

@Eomm
Copy link
Member Author

Eomm commented Apr 23, 2021

The error:

$ npm ls ajv
fastify@3.15.0 C:\Users\behem\workspaceFastify\fastify_2
├─┬ @fastify/ajv-compiler-8@npm:@fastify/ajv-compiler@0.0.1 (git+ssh://git@github.com/fastify/ajv-compiler.git#285644e79cff615a7e777f06e126fec8c11c367d)
│ └── ajv@8.1.0
├─┬ @fastify/ajv-compiler@1.0.0
│ └── ajv@6.12.6 deduped
├─┬ ajv-errors@1.0.1
│ └── ajv@6.12.6 deduped
├─┬ ajv-formats@2.0.2
│ └── ajv@6.12.6 deduped invalid
├─┬ ajv-i18n@3.6.0
│ └── ajv@6.12.6 deduped
├─┬ ajv-merge-patch@4.1.0
│ └── ajv@6.12.6 deduped
├── ajv@6.12.6
├─┬ coveralls@3.1.0
│ └─┬ request@2.88.2
│   └─┬ har-validator@5.1.5
│     └── ajv@6.12.6 deduped
├─┬ eslint@7.24.0
│ ├─┬ @eslint/eslintrc@0.4.0
│ │ └── ajv@6.12.6 deduped
│ ├── ajv@6.12.6 deduped
│ └─┬ table@6.5.1
│   └── ajv@8.1.0
├─┬ fast-json-stringify@2.6.0
│ └── ajv@6.12.6 deduped
├─┬ light-my-request@4.4.1
│ └── ajv@6.12.6 deduped
└─┬ standard@16.0.3
  └─┬ eslint@7.13.0
    ├─┬ @eslint/eslintrc@0.2.2
    │ └── ajv@6.12.6 deduped
    ├── ajv@6.12.6 deduped
    └─┬ table@5.4.6
      └── ajv@6.12.6 deduped

npm ERR! code ELSPROBLEMS
npm ERR! invalid: ajv@6.12.6 C:\Users\behem\workspaceFastify\fastify_2\node_modules\ajv

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\behem\AppData\Local\npm-cache\_logs\2021-04-23T20_24_26_250Z-debug.log

"@fastify/proxy-addr": "^3.0.0",
"abstract-logging": "^2.0.0",
"ajv": "^6.12.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still needed for the compiled options runtime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the build/build-validation.js script?
Is not it created with dev dependencies on a local env?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result still depends on ajv at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm missing something here:
the configValidator.js script is built from the build-validation.js to avoid the ajv loading when the user creates a new fastify instance.
The build-validation.js script is executed offline and without any automation and it is not executed when the user install the fastify module.
For these reasons I thought that move ajv to the devDependency was fine

Where am I wrong on this workflow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated script has a runtime dependency on some utilities within Ajv.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to bother you but I can't find any ajv reference excepts in /test and build-validation.js

I have run rm -rf node_modules/ && npm install --production and run a server with input parameters and it works

const fastify = require('./')({ keepAliveTimeout: 'not integer' })
fastify.listen(8080)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok. However we had fastify/light-my-request#115 (comment) in the past. Can you check if it'd also be the same for Ajv@8?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, i have checked: the output of the 4.4.0 that has the issue produce the validate.js that contains this line:

...
const func0 = require("ajv/dist/compile/equal").
...

While the output of the build-validation.js has not this statement

@Eomm
Copy link
Member Author

Eomm commented May 15, 2021

Tests are green, tsd is failing

cc @fastify/typescript

@Eomm Eomm marked this pull request as ready for review May 15, 2021 09:21
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work. Thank you for taking this on and sticking with it.

@Eomm Eomm mentioned this pull request May 17, 2021
4 tasks
@Eomm Eomm requested a review from zekth May 18, 2021 19:06
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

Anybody object on this landing?

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 5dfec54 into fastify:main May 21, 2021
@mcollina
Copy link
Member

Unfortunately this broke typescript support on pnpm and yarn.

@climba03003
Copy link
Member

For yarn, I think it should be problem of tsd or @types/pino.
We face the same problem before, it is about type inference cannot work properly inside tsd.

For pnpm, it is broken by this PR.

@Eomm
Copy link
Member Author

Eomm commented May 21, 2021

Fix ref #3084

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ajv@7 upgrade plan
7 participants