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

[4.0.1] Cannot find module fast-json-stringify/serializer #3998

Closed
2 tasks done
DRoet opened this issue Jun 11, 2022 · 23 comments · Fixed by #4004
Closed
2 tasks done

[4.0.1] Cannot find module fast-json-stringify/serializer #3998

DRoet opened this issue Jun 11, 2022 · 23 comments · Fixed by #4004

Comments

@DRoet
Copy link
Contributor

DRoet commented Jun 11, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the regression has not already been reported

Last working version

4.0.0

Stopped working in version

4.0.1

Node.js version

18.3.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Docker

💥 Regression Report

following error when starting my application:

node:internal/modules/cjs/loader:939
   const err = new Error(message);
               ^
 
 Error: Cannot find module 'fast-json-stringify/serializer'
 Require stack:
 - /app/node_modules/fastify/lib/error-serializer.js
 - /app/node_modules/fastify/lib/error-handler.js
 - /app/node_modules/fastify/lib/reply.js
 - /app/node_modules/fastify/fastify.js
     at Module._resolveFilename (node:internal/modules/cjs/loader:939:15)
     at Module._load (node:internal/modules/cjs/loader:780:27)
     at Module.require (node:internal/modules/cjs/loader:1005:19)
     at require (node:internal/modules/cjs/helpers:102:18)
     at Object.<anonymous> (/app/node_modules/fastify/lib/error-serializer.js:6:20)
     at Module._compile (node:internal/modules/cjs/loader:1105:14)
     at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
     at Module.load (node:internal/modules/cjs/loader:981:32)
     at Module._load (node:internal/modules/cjs/loader:827:12)
     at Module.require (node:internal/modules/cjs/loader:1005:19) {
   code: 'MODULE_NOT_FOUND',
   requireStack: [
     '/app/node_modules/fastify/lib/error-serializer.js',
     '/app/node_modules/fastify/lib/error-handler.js',
     '/app/node_modules/fastify/lib/reply.js',
     '/app/node_modules/fastify/fastify.js'
   ]
 }

Steps to Reproduce

Use simple hello world:

import Fastify from 'fastify'
const fastify = Fastify({
   logger: true,
})

fastify.get('/', async (request, reply) => {
   return { hello: 'world' }
})

await fastify.listen({ port: 3000 })

and start it using node app.js

Expected Behavior

No response

@DRoet
Copy link
Contributor Author

DRoet commented Jun 11, 2022

Hmm I am using yarn v3.2.1 and it seems as if it is not pulling version 4.1.0 of fast-json-stringify after upgrading my dependencies using yarn upgrade-interactive

"fast-json-stringify@npm:^4.0.0":
  version: 4.0.0
  resolution: "fast-json-stringify@npm:4.0.0"
  dependencies:
    ajv: ^8.10.0
    ajv-formats: ^2.1.1
    deepmerge: ^4.2.2
    fast-uri: ^2.0.0
    rfdc: ^1.2.0
    string-similarity: ^4.0.1
  checksum: be75cc7f87a8cf8623a135de5513aae80e39cdc4df7f9f3c735c686f77f501edd06fa8361026cd6be3628ac4e5ff2f5fe804a9ab11b9ffb828979965548b5af4
  languageName: node
  linkType: hard

@zedeus
Copy link

zedeus commented Jun 11, 2022

fastify/fast-json-stringify-compiler#12

@DRoet
Copy link
Contributor Author

DRoet commented Jun 11, 2022

Yup deleting the lock file does work, but like you said in the linked issue it's not a great solution.

@zedeus
Copy link

zedeus commented Jun 11, 2022

It's an unacceptable workaround imo, there's no reason to not bump the dependency.

@climba03003
Copy link
Member

It's an unacceptable workaround imo, there's no reason to not bump the dependency.

Even if it is bumped in package.json. There will likely be a version dedicated to just dependency bump.
That means your problem still exist.

If it is a production environment. I don't see a reason why you install one new update but not the others.
4.0.0 error serializer actually work just fine, but syntax error actually do not affect the code run.

@climba03003
Copy link
Member

climba03003 commented Jun 11, 2022

If it is an actual problem, I see fastify/fast-json-stringify#461 is a better option.
It remove fast-json-stringify from the generated code.

@mcollina
Copy link
Member

Update all your dependencies and it will resolved.

@woss
Copy link

woss commented Jun 12, 2022

not working on 4.0.1
image

full demo

import Fastify from 'fastify';

const fastify = Fastify({
  logger: true
});

fastify.get('/', async (request, reply) => {
  return { hello: 'world' };
});

/**
 * Run the server!
 */
const start = async () => {
  try {
    await fastify.listen({ port: 3000 });
  } catch (err) {
    fastify.log.error(err);
    process.exit(1);
  }
};
start();

Also, @mcollina why close this issue when your suggestion was not verified by the person who raised the issue?

@mcollina
Copy link
Member

As I said, running npm update will fix this. Unless there is something else I'm missing, in which case could you upload a repo where it is not working? thanks!

@woss
Copy link

woss commented Jun 12, 2022

@mcollina what npm update should update? all the packages? this is never a recommended solution and can lead to many issues. as you can see i have the versions fixed so nothing to update. the 4.0.1 is the latest version, shouldn't be fixed in that version?

Repro code is exactly as i showed. deps and the code, there is no need for the repo.

@Eomm
Copy link
Member

Eomm commented Jun 12, 2022

This issue is solved in fastify@4.0.1, which includes the fast-json-stringify@4.1.0
Ref PR: #3996

What is the npm ls fast-json-stringify output?

@woss
Copy link

woss commented Jun 12, 2022

@Eomm thanks for the answer, i don't use npm, i use pnpm + rushstack. You are correct, the fastify@4.0.1 indeed includes the
fast-json-stringify@4.1.0 via @fastify/fast-json-stringify-compiler 3.0.0

I've updated the fastify in one micro-service, the rest are on 3.x.

Please check the screenshot, this shouldn't be an issue since pnpm already solves the duplicates issues by correctly creating the paths. Could it be that the fastify has issues when having two different versions?

image

If you are not familiar with rushstack, it's like the workspace, each microservice has node_modules. Here is the example
image

@mcollina
Copy link
Member

I sincerely do not understand what the problem is. Please upload a repository that is failing with the latest version of all dependencies. We'll be happy to take a look.

I also do not understand the issue with "update all dependencies" and why you do not want to do this.

@zedeus
Copy link

zedeus commented Jun 12, 2022

fast-json-stringify@4.1.0 via @fastify/fast-json-stringify-compiler 3.0.0

That is not correct: https://github.com/fastify/fast-json-stringify-compiler/blob/main/package.json#L31
While Fastify itself has a dependency on fast-json-stringify, it's only a devDependency: https://github.com/fastify/fastify/blob/main/package.json#L149

This means we have a dependency tree that, in production, goes: fastify 4.0.1 -> fast-json-stringify-compiler 3.0.0 -> fast-json-stringify 4.0.0
The problem of course is that fastify relies on a feature introduced in fast-json-stringify 4.1.0, which is not available in the nested dependency. How is this not a problem? Why not just bump the dependency in fast-json-stringify-compiler to fix it? The fact that it works when you run npm update seems like a workaround at best.

@woss
Copy link

woss commented Jun 12, 2022

I also do not understand the issue with "update all dependencies" and why you do not want to do this.

@mcollina because most of the time npm update breaks things. if you do this that's fine but please don't promote that since it really is not the best solution.

@jsumners
Copy link
Member

The fact that it works when you run npm update seems like a workaround at best.

It's not "a workaround". It is the whole purpose of semver ranges. We will not be updating all modules everywhere every time a transitive dependency bumps a minor or patch version that is included in the qualifying semver range.

@jsumners
Copy link
Member

https://github.com/fastify/fast-json-stringify-compiler/blob/864956aede405c9408392945972fd985fa6e440d/package.json#L31 clearly shows that it will pull in fast-json-stringify@4.1.0 due to the ^ range qualifier.

Update your dependencies.

@Eomm
Copy link
Member

Eomm commented Jun 12, 2022

fast-json-stringify@4.1.0 via @fastify/fast-json-stringify-compiler 3.0.0

That is not correct: https://github.com/fastify/fast-json-stringify-compiler/blob/main/package.json#L31 While Fastify itself has a dependency on fast-json-stringify, it's only a devDependency: https://github.com/fastify/fastify/blob/main/package.json#L149

This means we have a dependency tree that, in production, goes: fastify 4.0.1 -> fast-json-stringify-compiler 3.0.0 -> fast-json-stringify 4.0.0 The problem of course is that fastify relies on a feature introduced in fast-json-stringify 4.1.0, which is not available in the nested dependency. How is this not a problem? Why not just bump the dependency in fast-json-stringify-compiler to fix it? The fact that it works when you run npm update seems like a workaround at best.

I agree with the analysis but it is not necessary to publish a new release.

Did you try the pnpm update fast-json-stringify command?

@zedeus
Copy link

zedeus commented Jun 12, 2022

npm update fast-json-stringify works nicely without affecting all other packages, thanks!

@simoneb
Copy link
Contributor

simoneb commented Jun 13, 2022

I agree with @zedeus. This IS an issue in fastify. #3996 introduced a production dependency on fast-json-stringify (see these lines), and more to that, to a specific version (4.0.1) of that library. If fastify now depends on it, it should be declared in fastify's production dependencies rather than assuming that it will come in some form through a transitive dependency.

Fixed in #4004

@climba03003
Copy link
Member

I agree with @zedeus. This IS an issue in fastify. #3996 introduced a production dependency on fast-json-stringify (see these lines), and more to that, to a specific version (4.0.1) of that library. If fastify now depends on it, it should be declared in fastify's production dependencies rather than assuming that it will come in some form through a transitive dependency.

Fixed in #4004

I would see fastify/fast-json-stringify#461 to be merged and publish.
And we update the error-serializer.js again.

@mcollina
Copy link
Member

I agree with @zedeus. This IS an issue in fastify. #3996 introduced a production dependency on fast-json-stringify (see these lines), and more to that, to a specific version (4.0.1) of that library. If fastify now depends on it, it should be declared in fastify's production dependencies rather than assuming that it will come in some form through a transitive dependency.

Fixed in #4004

This is correct. Sorry for the issue and the great write up @zedeus and @simoneb!

--

I would see fastify/fast-json-stringify#461 to be merged and publish.
And we update the error-serializer.js again.

Let's fix it first.

@jsumners
Copy link
Member

This IS an issue in fastify. #3996 introduced a production dependency on fast-json-stringify (see these lines), and more to that, to a specific version (4.0.1) of that library.

Thank you for linking to the actual code. We definitely want to add direct dependencies to the dependencies list.

I think we really need a CI step that does:

  1. pnpm install --production or npm install --legacy-deps --legacy-peer-deps
  2. node some_basic_server.js
  3. curl /several_endpoints

That should catch these cases in the future as neither of the options in step one will install the transitive dependency in such a fashion that Fastify itself will have access to it.

trentm added a commit to elastic/apm-agent-nodejs that referenced this issue Jun 13, 2022
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 a pull request may close this issue.

9 participants