search

log4js-node / streamroller

node.js file streams that roll over when they reach a maximum size, or a date/time.
MIT License
37 stars 19 forks source link

Incompatible versions of fs-extra specified for streamroller and log4js #170

Closed cmckni3 closed 7 months ago

cmckni3 commented 10 months ago

I am running into incompatible package versions of fs-extra across 3 packages. log4js, streamroller, and jest-openapi.

The same error (cannot read property name of undefined) in log4js issue 1225 occurs when running jest tests.

I see streamroller is still using fs-extra 8.x where log4js is using fs-extra 11.x. jest-openapi is using fs-extra 9.x.

Currently, I specified an override in the package.json file to force fs-extra 9.x to be used for all packages which appears to be a short-term fix.

Should streamroller's fs-extra package be updated to align with log4js?

I believe aligning them would fix https://github.com/log4js-node/log4js-node/issues/1225

lamweili commented 10 months ago

The error (cannot read property name of undefined) by fs-extra only occurred in fs-extra@10.0.x. It was then patched in fs-extra@10.1.0 under https://github.com/jprichardson/node-fs-extra/pull/953 which was part of https://github.com/log4js-node/log4js-node/issues/1225.

I'm sceptical because the affected version is not in use here:

Furthermore, all log4js unit tests passed.

So, I need more information from you to replicate and fix the issue.

lamweili commented 10 months ago

Or did you check if any other dependencies you have might have that pulled in fs-extra@10.0.x?

cmckni3 commented 10 months ago

The error (cannot read property name of undefined) by fs-extra only occurred in fs-extra@10.0.x. It was then patched in fs-extra@10.1.0 under jprichardson/node-fs-extra#953 which was part of log4js-node/log4js-node#1225.

Makes sense but failing in my repo with fs-extra@8.1.0 and fs-extra@9.1.0 installed at the same time.

streamroller is using fs-extra@^8.1.0 as part of dependencies.

Hence my question: should streamroller be using the same major version of fs-extra as log4js?

So, I need more information from you to replicate and fix the issue.

  • What is your Node.js and log4js version?
  • I would also need a simple, stripped-down version of your existing application's jest unit tests and the package.json.
  • A stack-trace would be good as well.

Thought I posted in the issue, but I am using log4js version 6.9.1 with node.js 18.18.2.

I can try to reproduce but probably won't have time. This is in a client project. The error in the project has the same stacktrace as https://github.com/log4js-node/log4js-node/issues/1225. Personally, I don't like the npm cli for projects to eliminate potential issues with transitive dependencies but I have no choice.

My workaround right now is to force resolve fs-extra to 9.x or 11.x.

cmckni3 commented 10 months ago

Or did you check if any other dependencies you have might have that pulled in fs-extra@10.0.x?

Checked again and only see 9.1.0 and 8.1.0 installed.

Doesn't make sense to me either tbh

lamweili commented 10 months ago

Thought I posted in the issue, but I am using log4js version 6.9.1 with node.js 18.18.2.

Apparently, you didn't 😓. But it's all good now.

Hence my question: should streamroller be using the same major version of fs-extra as log4js?

Nope. streamroller should use fs-extra@^8.1.0 as it has to maintain compatibility with Node.js 8. If need be, there must be a major semver release.

https://github.com/log4js-node/streamroller/blob/8098400a0d4b8b330500e5e6241817453f858577/package.json#L44-L51

log4js merely uses fs-extra@^11.1.0 as one of the devDependencies for unit testing purposes. Alternatively, I can always downgrade that to match streamroller since it is devDependencies and won't affect runtime.

Checked again and only see 9.1.0 and 8.1.0 installed.

As you mentioned, you only have fs-extra 8.1.0 and 9.1.0 installed. Thus, it has nothing to do with fs-extra@^11.1.0 (or log4js).

Do you know which dependency pulled in fs-extra@9.1.0 in that case? Perhaps you can upload your package.json so we can help to eyeball the dependencies?

lamweili commented 10 months ago

I can try to reproduce but probably won't have time.

Perhaps to help narrow down the cause, can you do the following to your index.js? This should be a fast diagnostic.

First, log out all the functions before you do any require/import. Then, require/import all your dependencies but not log4js and log out the functions again. Lastly, require/import streamroller (I prefer streamroller instead of log4js here to find the issue).

Code logic can be after all these; it won't need to get there to trigger the error.

console.log("=====1=====");
console.log("NODEJS VERSION:", process.version);
console.log("2nd-level API fs.appendFile:", fs.appendFile);           // an arbitrary second-level API (fs.xxx)
console.log("2nd-level API fs.realpath:", fs.realpath);
console.log("3rd-level API fs.realpath.native:", fs.realpath.native); // a third-level API (fs.xxx.yyy)

// your existing require/import here (code logic not required)

console.log("=====2=====");
console.log("2nd-level API fs.appendFile:", fs.appendFile);           // an arbitrary second-level API (fs.xxx)
console.log("2nd-level API fs.realpath:", fs.realpath);
console.log("3rd-level API fs.realpath.native:", fs.realpath.native); // a third-level API (fs.xxx.yyy)

console.log("=====3=====");
var streamroller = require("streamroller");                           // you should get the error stacktrace here

console.log("=====4=====");                                           // if no error stacktrace, proceed
console.log("2nd-level API fs.appendFile:", fs.appendFile);           // an arbitrary second-level API (fs.xxx)
console.log("2nd-level API fs.realpath:", fs.realpath);
console.log("3rd-level API fs.realpath.native:", fs.realpath.native); // a third-level API (fs.xxx.yyy)

// your existing code logic
cmckni3 commented 10 months ago

Thought I posted in the issue, but I am using log4js version 6.9.1 with node.js 18.18.2.

Apparently, you didn't 😓. But it's all good now.

Hence my question: should streamroller be using the same major version of fs-extra as log4js?

Nope. streamroller should use fs-extra@^8.1.0 as it has to maintain compatibility with Node.js 8. If need be, there must be a major semver release.

https://github.com/log4js-node/streamroller/blob/8098400a0d4b8b330500e5e6241817453f858577/package.json#L44-L51

log4js merely uses fs-extra@^11.1.0 as one of the devDependencies for unit testing purposes. Alternatively, I can always downgrade that to match streamroller since it is devDependencies and won't affect runtime.

Checked again and only see 9.1.0 and 8.1.0 installed.

As you mentioned, you only have fs-extra 8.1.0 and 9.1.0 installed. Thus, it has nothing to do with fs-extra@^11.1.0 (or log4js).

Do you know which dependency pulled in fs-extra@9.1.0 in that case? Perhaps you can upload your package.json so we can help to eyeball the dependencies?

I see streamroller is still using fs-extra 8.x where log4js is using fs-extra 11.x. jest-openapi is using fs-extra 9.x.

jest-openapi 0.14.2

cmckni3 commented 10 months ago

As you mentioned, you only have fs-extra 8.1.0 and 9.1.0 installed. Thus, it has nothing to do with fs-extra@^11.1.0 (or log4js).

Ok, didn't really say that. I was merely suggesting aligning the major version of fs-extra across streamroller and log4js

Causes issues in my project's test suite because of the version difference. The application works fine but its jest test suite breaks. Don't know why as you stated before my project is not using nor installing fs-extra 10.x. I was merely suggesting aligning both streamroller and log4js to 11.x to prevent the issue that I mentioned.

cmckni3 commented 10 months ago

I can try to reproduce but probably won't have time.

Perhaps to help narrow down the cause, can you do the following to your index.js? This should be a fast diagnostic.

First, log out all the functions before you do any require/import. Then, require/import all your dependencies but not log4js and log out the functions again. Lastly, require/import streamroller (I prefer streamroller instead of log4js here to find the issue).

Code logic can be after all these; it won't need to get there to trigger the error.

console.log("=====1=====");
console.log("NODEJS VERSION:", process.version);
console.log("2nd-level API fs.appendFile:", fs.appendFile);           // an arbitrary second-level API (fs.xxx)
console.log("2nd-level API fs.realpath:", fs.realpath);
console.log("3rd-level API fs.realpath.native:", fs.realpath.native); // a third-level API (fs.xxx.yyy)

// your existing require/import here (code logic not required)

console.log("=====2=====");
console.log("2nd-level API fs.appendFile:", fs.appendFile);           // an arbitrary second-level API (fs.xxx)
console.log("2nd-level API fs.realpath:", fs.realpath);
console.log("3rd-level API fs.realpath.native:", fs.realpath.native); // a third-level API (fs.xxx.yyy)

console.log("=====3=====");
var streamroller = require("streamroller");                           // you should get the error stacktrace here

console.log("=====4=====");                                           // if no error stacktrace, proceed
console.log("2nd-level API fs.appendFile:", fs.appendFile);           // an arbitrary second-level API (fs.xxx)
console.log("2nd-level API fs.realpath:", fs.realpath);
console.log("3rd-level API fs.realpath.native:", fs.realpath.native); // a third-level API (fs.xxx.yyy)

// your existing code logic

I can attach the debugger when I have a chance. Might not be for 2 weeks though.

cmckni3 commented 10 months ago

Nope. streamroller should use fs-extra@^8.1.0 as it has to maintain compatibility with Node.js 8.

Why? node.js 8.x has been end of life since January 1, 2020.

cmckni3 commented 10 months ago

Thought I posted in the issue, but I am using log4js version 6.9.1 with node.js 18.18.2.

Apparently, you didn't 😓. But it's all good now.

Hence my question: should streamroller be using the same major version of fs-extra as log4js?

Nope. streamroller should use fs-extra@^8.1.0 as it has to maintain compatibility with Node.js 8. If need be, there must be a major semver release. https://github.com/log4js-node/streamroller/blob/8098400a0d4b8b330500e5e6241817453f858577/package.json#L44-L51

log4js merely uses fs-extra@^11.1.0 as one of the devDependencies for unit testing purposes. Alternatively, I can always downgrade that to match streamroller since it is devDependencies and won't affect runtime.

Checked again and only see 9.1.0 and 8.1.0 installed.

As you mentioned, you only have fs-extra 8.1.0 and 9.1.0 installed. Thus, it has nothing to do with fs-extra@^11.1.0 (or log4js). Do you know which dependency pulled in fs-extra@9.1.0 in that case? Perhaps you can upload your package.json so we can help to eyeball the dependencies?

I see streamroller is still using fs-extra 8.x where log4js is using fs-extra 11.x. jest-openapi is using fs-extra 9.x.

jest-openapi 0.14.2

I see 9.x is a devDependency in jest-openapi as well. Not sure why the error is when log4js is required in the jest test suite.

Initially, I thought the stacktrace in the project was wrong in CI but reproduced on local dev machine.

Let me dig some more.

lamweili commented 10 months ago

I see streamroller is still using fs-extra 8.x where log4js is using fs-extra 11.x. jest-openapi is using fs-extra 9.x.

jest-openapi 0.14.2

Technically, log4js is not using fs-extra. By "using", I define it as during runtime. It is under devDependencies solely for unit tests.

I can attach the debugger when I have a chance. Might not be for 2 weeks though.

Thanks for providing jest-openapi 0.14.2 as one of the dependencies. I'll do a quick check on it. You beat me to it.

I see 9.x is a devDependency in jest-openapi as well. Not sure why the error is when log4js is required in the jest test suite.

Can you provide the dependencies and devDependencies section of package.json as well? So that I can also take a look at any other possible "culprits".

Nope. streamroller should use fs-extra@^8.1.0 as it has to maintain compatibility with Node.js 8.

Why? node.js 8.x has been end of life since January 1, 2020.

Because it would have to be a major semver change for streamroller to drop off any older Node.js support. And that might also cause a chaining major semver upgrade for log4js and so on.

cmckni3 commented 10 months ago

Can you provide the dependencies and devDependencies section of package.json as well? So that I can also take a look at any other possible "culprits".

Sure

dependencies:

{
  "dependencies" : {
    "@aws-sdk/client-athena": "^3.0.0",
    "@aws-sdk/client-lambda": "^3.0.0",
    "@aws-sdk/client-s3": "^3.0.0",
    "@aws-sdk/client-secrets-manager": "^3.0.0",
    "@aws-sdk/client-ssm": "^3.0.0",
    "@okta/jwt-verifier": "^3.0.0",
    "@vendia/serverless-express": "^4.0.0",
    "cookie-parser": "^1.0.0",
    "cors": "^2.0.0",
    "express": "^4.0.0",
    "http-errors": "^2.0.0",
    "log4js": "^6.0.0",
    "swagger-autogen": "^2.0.0",
    "yaml": "^2.0.0"
  }
}
{
  "devDependencies": {
    "@aws-sdk/util-stream-node": "^3.0.0",
    "aws-sdk-client-mock": "^3.0.0",
    "aws-sdk-client-mock-jest": "^3.0.0",
    "chai": "^4.0.0",
    "env-cmd": "^10.0.0",
    "jest": "^29.5.0",
    "jest-openapi": "~0.14.2",
    "nodemon": "^3.0.0",
    "supertest": "^6.0.0"
  }
}

Need to log off for the night. Might have missed some packages in dependencies though. Will update tomorrow if I have a chance.

lamweili commented 10 months ago

Hmm, I created a sample.zipproject using your dependencies and devDependencies.

I required/imported both the dependencies and devDependencies in the index.js for runtime. There isn't any error. 🤔

image

cmckni3 commented 9 months ago

Hmm, I created a sample.zipproject using your dependencies and devDependencies.

I required/imported both the dependencies and devDependencies in the index.js for runtime. There isn't any error. 🤔

image

Circling back. I updated the dependencies in my original comment with some missing dependencies.

Frustrating that I can't copy and paste the dependencies from the project. Also, frustrating that it seems like you don't believe me.

I will have to look at the zip file when I have a chance. It happens during jest runs.

lamweili commented 9 months ago

Circling back. I updated the dependencies in my original comment with some missing dependencies.

Frustrating that I can't copy and paste the dependencies from the project. Also, frustrating that it seems like you don't believe me.

I will have to look at the zip file when I have a chance. It happens during jest runs.

I have updated the sample.zip with your updated dependencies from https://github.com/log4js-node/streamroller/issues/170#issuecomment-1811618041. Also, added a random jest test so that we can do a jest run.

node run image

jest run image

It is equally frustrating not to be able to replicate the issue.

cmckni3 commented 7 months ago

Circling back. I updated the dependencies in my original comment with some missing dependencies. Frustrating that I can't copy and paste the dependencies from the project. Also, frustrating that it seems like you don't believe me. I will have to look at the zip file when I have a chance. It happens during jest runs.

I have updated the sample.zip with your updated dependencies from #170 (comment). Also, added a random jest test so that we can do a jest run.

node run image

jest run image

It is equally frustrating not to be able to replicate the issue.

Trying this out. Unfortunately, no longer have access to the code but let me see what happens locally with the project you attached.

cmckni3 commented 7 months ago

No idea. Might have missed a package. Attached a jest-openapi sample. Issue does not exist using it either.

sample.zip

Closing. Apologies for the waste of time. Very strange issue.