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

[@feathersjs/schema] schema function is not pure and crashes in ES Modules #2681

Closed
FossPrime opened this issue Jun 30, 2022 · 6 comments
Closed

Comments

@FossPrime
Copy link
Member

FossPrime commented Jun 30, 2022

Steps to reproduce

  • Call schema twice with the same parameters.

Expected behavior

Don't crash.

Actual behavior

"schema with key or id "UserPatch" already exists"

System configuration

Module versions: dove pre
Module Loader: esbuild

Reproduction:

use schema instead of safeSchema, wait for server restart, than modify a comment string. When the server is being restarted the bug will crash esbuild wasm as we are building in the same context and something global is conflicting... this might be unavoidable, so I propose wrapping schema with my fix when appropriate.

https://stackblitz.com/edit/bug-feathersjs-schema-2680?file=src%2Fschema%2Fusers.schema.ts,src%2Futils.ts

@daffl
Copy link
Member

daffl commented Jul 7, 2022

I would see how AJV does this since the error is coming from there. We could work around it but I'm pretty sure someone needed to do hot module reloading with AJV before.

@FossPrime
Copy link
Member Author

FossPrime commented Jul 20, 2022

I've ruled out upstream... I cannot reproduce the issue with json-schema-to-ts@2.5.4 and vite@3.0.1

https://stackblitz.com/edit/vitejs-vite-qdrpwy?file=BugReproduction.ts

With up to date @feathersjs/schema dependencies, I can no longer reproduce the issue at all!

https://stackblitz.com/edit/bug-feathersjs-schema-2680-vfz5ho?file=src%2Fschema%2Fusers.schema.ts,src%2FsafeSchema.ts,client.ts

@FossPrime
Copy link
Member Author

The issue only manifests when loading a module from node_modules, if the module is anywhere else in the project, DEFAULT_AJV will be discarded, cleaning up any polluting modules. See PR.

@FossPrime FossPrime reopened this Jul 21, 2022
@FossPrime
Copy link
Member Author

FossPrime commented Jul 22, 2022

Other situations I can think of where this is a problem:

  • Running two feathers instances in the same context and both of them have a User schema.
  • Running tests on Schemas, you have to be careful to not name a schema the same as another test if the context is shared.
  • dynamically creating schemas from a feathers client
  • Monorepos where all projects are expected to run in the same node context
  • Clients that connect to multiple feathers servers

Normally we'd use the Feathers App's context... But that would probably break intellisence as the types couldn't be computed at ES Module compile time. Types are already slow to compile because App.ts has no constructor, so typescript can't process types without attempting to execute everything; Rather than processing only top level stateless statements that you'd expect when calling import './src/app.ts'

@daffl
Copy link
Member

daffl commented Aug 30, 2022

This should be fixed in the next prerelease via #2702. Thank you for the PR!

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

No branches or pull requests

2 participants