-
Notifications
You must be signed in to change notification settings - Fork 87
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
Making callSiteLocation more robust #48
base: master
Are you sure you want to change the base?
Conversation
Can you add a test for the environment where it is a string or provide a reproducable test case? As far as I know, that API from v8 cannot return a string, so perhaps there is something very wrong in the environment. I can add support for more environments, but need to add tests and understand the environment so it can be supported. I won't just merge this without that. |
I tried Node.js 5 manually, and there is 5.12 as part of the CI run and everything works. It could be the specific version of NodeJS 5 if you happen to know I can try it. |
Ok, I'm trying to investigate a bit further... So far I found out that
Note that I'm under Windows This seems to match what I see: https://stackoverflow.com/a/67073333/636849 |
I am also on and testing in Windows, and the CI tests 5.12.0 also in Windows via AppVeyor... |
Also in case you are curious, this is the docs for the It does seem weird if it is not working as v8 documents; I wonder what the cause / environment would be. Maybe a custom-compiled Node.js you are using? |
After re-reading your code and the documentation on I don't quite understand why in your lib code |
Yes, that is the default behavior of |
OK, thanks for the explanation. I think 2 libs are conflicting in my case: https://github.com/felixge/node-stack-trace/blob/v0.0.10/lib/stack-trace.js#L8 |
Interesting. It shouldn't, since you see it sets Edit: The |
You are right, this was a red herring I found just out that my bundle also integrates some |
This is definitively an issue with this polyfill: https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/wrap-error-constructor-with-cause.js |
Interesting. There is definitely a lot going on in there and honestly I am not very familiar with core-js 😂 . Do you think you can put together a simple like file that combines that core-js stuff that reproduces the error? I can help take a look / even help make any appropriate tests if needed. Looking at the code, the bulk of it only runs with "FORCED" is true, which sounds like something has to "force" core-js to do whatever the wrapper stuff is (assuming that is where the conflict is coming from). It does seem like such a polyfill should be unnecessary for Node.js 5.12.0, though (besides the part above the "FORCED" which adds |
There is some minimal project that reproduces the bug I'm facing: (I misclicked and closed this by error - reopening now) |
I think a better fix than this PR could be to ensure that in Also, I tried to make Maybe you could also try to simply use an existing lib to do the job, like https://github.com/felixge/node-stack-trace |
That sounds fine. I am happy to accept a PR that would do so 😊
A goal of this module is to have zero dependencies. In addition, the parsing of stack traces in there does not work correctly in Node.js, only the web browser, unfortunately. For example, file names can include a newline character unlike a web page address, which you break the string parsing of the stack that module does when done in Node.js. |
P.S. thanks for that repo; I will use it to help dig in to a solution as well 😀 |
Hi @Lucas-C I'm not sure if I'm missing something here, but I tried to run your repo but I couldn't even install the modules. I am using Node.js 5.12.0, as I believe that is what you said to use. Let me know if I should be using something different:
I tried to run The error leads me to believe that it doesn't work with Node.js 5.12.0, though I thought that is what you said you are running. How did you get it to work / am I still supposed to be using Node.js 5.12.0 with your repo? Edit: oh, nvm, I see now it shows in your readme you use Node.js 14 to do the build, and the run it in 5.12. |
Exactly 😊 I updated this PR with the "improved" solution I suggested earlier |
Awesome, thank you! The million-dollar question, of course, is does the solution actually work besides just not get the error. Like, do you still get the deprecation messages? Is there breakage elsewhere when other things are called? We should get the existing test suite to run in this new environment to validate the functionality of this module with these fixes 👍 |
Agreed I could add a unit test that applies the same polyfill as |
Maybe. Hard to really know until I see it, though 😂 |
Ok. I'll try to craft one soon, but this won't be high priority for me now, so feel free to replace this PR by some other solution you wrote yourself, with tests that match your style |
NP 👍 even if you end up leaving it here (hopefully not?), this was a HUGE help! The repo itself was so helpful; and your start here is very helpful. I'll play around as well to get something landed; probably after work today or such. Have a good one!! |
Hi 👋 so great news: this can actually be easily fixed in This is going to be the best way forward, because even if you just limit the scope to APIs documented on the Node.js docs, it does in fact break https://nodejs.org/dist/latest-v16.x/docs/api/errors.html#errorstacktracelimit (you'll notice in code if you write Essentially the fix is that around the part were their wrapper copies properties from the original Effectively just the following (added in Object.defineProperties(WrappedError, {
prepareStackTrace: {
get: function () { return OriginalError.prepareStackTrace; },
set: function (v) { OriginalError.prepareStackTrace = v; }
},
stackTraceLimit: {
get: function () { return OriginalError.stackTraceLimit; },
set: function (v) { OriginalError.stackTraceLimit = v; }
}
}); |
I'll try to open a PR for |
I've a added one last commit with some starting point for a new test matrix entry I see you latest comment now: great that you found a fix for |
You deserve a lot of thanks as well for putting together that repo!! I don't think they intended to break this stuff; the only reason they did was because to add the new |
Note that "hardening" And I think that the patch I suggested on Plus it may be useful to have a test with |
Yea, you can open a PR that adds just a test if you want. As far as hardening, I mean if your changes actually work with all the features, sure. Unless there is a flood of users coming in to report this issue, it is likely better solved fixing upstream and letting it be vs adding additional complexity here, I think. But if you can come up with a fully working implementation, I'll land it. I can reopen this PR if you like, but with the fix being in Edit: it looks like Github actions refuses to run the PR due to invalid yaml; I tried to rerun it a few times. |
38e16c7
to
f7cfaa5
Compare
265f344
to
b4de4bf
Compare
The tests suite won't pass because it contains many assertions like this: function callold () { mylib.old() }
var stderr = captureStderr(callold)
assert.ok(stderr.indexOf(basename(__filename)) !== -1) But in this context, with the fix contained in this PR, the deprecation message does not include the source file: The GitHub Actions pipeline is now properly running but failing in this case due to those assertions: I don't see an easy way to fix them. Would you have any suggestion? |
Hi @Lucas-C thanks for the update. Yea, that was the main purpose of this module compared to others (and the deprecation stuff that is now built in to Node.js), as it's very difficult to understand where the deprecation message is coming from when a user upgrades something and their code base is dozens of files and thousands of lines. Having the stack right there definitely makes it so much easier to diagnose the code :) As for suggestions, I'll have to take a look. Unfortunately last night I had a really bad fall and my shoulders and right arm / hand is really messed up. Got some temp splints and stuff from the Dr. but it's mainly really painful. I went and opened zloirock/core-js#1061 this morning for you so while I'm probably going to be away from GitHub for some time, someone may be inclined to get the |
I also tried another approach here: sncf-connect-tech@852f04f (branch makeCallSite) The pipeline is fully ✔️: https://github.com/voyages-sncf-technologies/nodejs-depd/runs/5602759731 |
I'm sorry to hear that. |
Hi
We had several issues when using the
body-parser
lib, that itself usesdepd
, under NodeJS 5:This patch makes sure that
callSiteLocation
never crashes, even when some underlying methods do not exist or itscallSite
argument does not have the expected object type (it wasString
in some cases we investigated).