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

Avoid loading validator un-necessarily #718

Merged
merged 2 commits into from
May 16, 2024
Merged

Conversation

brettwillis
Copy link
Contributor

The validator module pulls in the entire ajv dependency and several others and is not always used depending on the condition. Avoid loading this module in generated code when not necessary.

Checklist

The validator module pulls in the entire `ajv` dependency and several others and is not always used depending on the condition. Avoid loading this module in generated code when not necessary.

Signed-off-by: Brett Willis <brettjohnwillis@gmail.com>
Copy link
Member

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

lgtm, except the part where it requires dependencies from other places than the fast-json-stringify/lib/standalone module. It's easier for us to ship changes in the dependencies in that way. Can you keep all demendencies inside the fast-json-stringify/lib/standalone and export them dynamically instead?

From:

module.exports.dependencies = {
  Serializer: require('./serializer'),
  Validator: require('./validator')
}

To:

module.exports.dependencies = {
  get Serializer() { return require('./serializer') },
  get Validator() { return require('./validator') }
}

@brettwillis
Copy link
Contributor Author

brettwillis commented May 15, 2024

Ok, while yes that would still be a worthwhile improvement on the current state, it has the following drawbacks versus my initial suggestion

  1. The generated code includes the code-generation code (i.e. the buildStandaloneCode function is included from ./lib/standalone when it is not needed in the generated code).
  2. Being a CJS module, bundlers like esbuild cannot tree-shake it so all of ./lib/validator and is dependences (ajv, ajv-formats, rfdc, fast-uri and their dependencies) will still end up in the bundled code.

As you may gather I'm coming from the perspective of a bundled build and I'm concerned about the huge amount of dependency code that get's pulled in to the bundle just by this one line of generated code.

It's easier for us to ship changes in the dependencies in that way.

I don't fully understand the history/implications there but as we're in full control of the generated code, isn't it easy to control the dependencies that are imported? It seems like the dependencies are already nicely controlled and isolated in ./lib/validator and ./lib/serializer.

Copy link
Member

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

I see. That makes sence for bundling.

I don't fully understand the history/implications there but as we're in full control of the generated code, isn't it easy to control the dependencies that are imported? It seems like the dependencies are already nicely controlled and isolated in ./lib/validator and ./lib/serializer.

The main issue here is that we don't require user to use the same lib version for standalone code generation and standalone code execution. This should work for all version under the same major. But anyway, it doesn't look like we've had a lot of changes in the standalone dependencies for a while, so I think we can change it.

lgtm after fixing the linting issue

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 mcollina merged commit c873689 into fastify:master May 16, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants