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: standalone mode #460

Merged
merged 2 commits into from
Jun 10, 2022
Merged

feat: standalone mode #460

merged 2 commits into from
Jun 10, 2022

Conversation

climba03003
Copy link
Member

  1. move debugMode in mode option.
  2. provide standalone mode which people can create the code that can directly require and run
  3. fixes breaking fastify standalone error serializer build #459

Checklist

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.

As I understand, we need this when we want to generate stringify function for schemas that we know before runtime.

  1. IMHO, it's better to create util that will use fjs to create such modules. I don't really like that one function can return a function or code of a module depending on the params.

  2. Can't we create these stringify functions in the runtime? We need to do it only once. I don't think it will affect performance significantly and, more importantly, we won't have to make sure we recompile the module after we fix something in fjs.

If I missed something, please tell me.

@climba03003
Copy link
Member Author

Can't we create these stringify functions in the runtime?

We always can do it.

I don't think it will affect performance significantly

I think it will affect cold start time.

@climba03003 climba03003 requested a review from Eomm June 10, 2022 21:28
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 658411c into master Jun 10, 2022
@mcollina mcollina deleted the standalone-mode branch June 10, 2022 21:40
@ivan-tymoshenko
Copy link
Member

@climba03003 @mcollina I was writing a long description of why it's not the best solution. Partially my fault, I should have requested changes, probably. The question is what can we do with it now?

This solution brings a dependency on internal code structure/names. It will block a lot of optimizations that will not support function serialization.

The first thing that will not work that I found is ajv schema validation because we use uuids when inserting a schema and then we will regenerate them.

const schemaKey = i.$id || randomUUID()

@ivan-tymoshenko
Copy link
Member

These are very serious changes, that would take a lot of effort to maintain. I guess we can fix the Fastify issue easier right now. Can we do something with it and then think deeply about what to do with this idea?

@climba03003
Copy link
Member Author

climba03003 commented Jun 10, 2022

I think we focus on fixing the fastify error first.
Since fastify do not use $ref in that code. We are safe from it currently.

Of course, we can provide a truely fast-json-stringify dependent free for standalone.
But, it require a bit longer time to work on.

@ivan-tymoshenko
Copy link
Member

@climba03003 can you just rebuild error schema serializer using the last fjs version that did it correctly?

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Jun 10, 2022

I think we focus on fixing the fastify error first.

I agree this is a reasonable approach, but creating a new API is serious. We can't just deprecate it if we find a better solution. We will have to support it.

@climba03003
Copy link
Member Author

climba03003 commented Jun 10, 2022

We can't just deprecate it if we find a better solution. We will have to support it.

Yes, it is. Can we add a experimental flag to indicate the user do not use it in code at this moment or it may break somehow.
Since, we are the package maintainer for fast-json-stringify and fastify.
I think we are safe to use it first and we know the limitation of it.

@mcollina
Copy link
Member

The original refactoring should not have shipped as they broke an important use case for Fastify. I'm happy to remove this flag in a semver-major change (if it's the only change) and update Fastify accordingly.

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.

breaking fastify standalone error serializer build
3 participants