dougwilson / nodejs-depd

Deprecate all the things
MIT License
325 stars 87 forks source link

Call-site calculation does not fail gracefully when Stack information is unavailable #27

Closed arciisine closed 6 years ago

arciisine commented 6 years ago

When the number of stack frames in an error is under 3, depd does not fail gracefully. I'm working on a framework that cleans up stack traces by hiding lines that are primarily boilerplate. In this process, this ends up at times with a stack trace that is small enough to cause this scenario.

Depd is a transitive dependency that I do not control, and so my application will break with imports failing almost seemingly without a pattern.

I'm happy to submit a PR. I'm thinking it should continue properly if it cannot retrieve stack information, but with just an unknown location.

arciisine commented 6 years ago

As an addendum, attempting to use NO_DEPRECATION only seems to affect whether or not something is logged, vs whether or not the information is calculated. By allowing NO_DEPRECATION to actually bypass the calculation, that would also work for my use case.

dougwilson commented 6 years ago

Weird, I remember some kind of fix along those lines, but not 100% sure. Is the transitive dependency in your tree the most up-to-date version (just to check in case you're mixing any such fix)? As for the NO_DEPRECATION, that's the correct behavior. I think the section on it in the readme does a pretty good job saying it just controls the output to STDERR and nothing else; the events are still emitted on process. If you think the readme can be clarified around that that would be cool to do too.

Yea, if you want to make a PR that would be cool, though if you can provide a repo that reproduces the issue first that I can look at, that would help a lot to get direction on the change prior to spending time working on it 👍

arciisine commented 6 years ago

Let me get a repro to you, and then we an see if a PR is worthwhile.

dougwilson commented 6 years ago

Closing since I never got a reproduction case or a follow up pull request.

charandas commented 6 years ago

@dougwilson this is happening for me when I am using sequelize with jest (ts-jest), with the special case that the wrapping library calling sequelize is using esm.

I basically had to disable my tests couple weeks ago because of this. I may be able to get you the repro.

Basically, once the stack frames become invalid, sequelize using depd for deprecating things makes sequelize un-usable in my tests.

Can I reopen this when I have the repro ready?

dougwilson commented 6 years ago

Yes, I just didn't want this hanging open indefinitely. I haven't heard anything from you in months, and it's not uncommon for folks to just vanish. I would say that is probably the best next steps: a new issue that includes reproduction steps where I can try to debug through it or a pull request with the fix + tests if you're up for it 👍

charandas commented 6 years ago

Thanks. Just for context, I am not the OP.

This is the line where improper stackframes just make the code fail with cannot get property of undefined, something along those lines.

https://github.com/dougwilson/nodejs-depd/blob/master/index.js#L110

Sure I will open another issue when I am ready.

dougwilson commented 6 years ago

Also, I see you mentioned sequelize. I'm not sure if it's causing the issue, but sequelize is using this module incorrectly. See this part of the README:

this module uses the calling file to get the boundary for the call stacks, so you should always create a new deprecate object in each file and not within some central file.

The sequelize module is incorrectly creating a single instance in a centralized file and sharing it across the code base, exactly what can cause issues when that is done.

charandas commented 6 years ago

Ah! I will look into this more deeply. Thank you.

dougwilson commented 6 years ago

No problem 👍 I cannot confirm that the incorrect usage is causing your particular issue, but it does cause stack trace-related issues since the frame the instance captured may not contain any of the same frames when the function is later called to display a message. It just sounds like that may be the culprit here, but not sure.

dougwilson commented 6 years ago

P.S. Sorry for confusing you with OP; I use email for everything here, basically read through email and click through the site to respond; this meant your icon was not next to the text as I was reading it (because I read in my email) and as I scrolled by they had the same colors so I incorrectly assumed it was the same user :D

charandas commented 6 years ago

No problem.

I think in my stack, the failure happens when sequelize tries to call the depd constructor itself while newing it.

When you say the function cannot find stack frames, it sounds to me like it's in the later workflow when depd instance is already constructed.

dougwilson commented 6 years ago

When you say the function cannot find stack frames, it sounds to me like it's in the later workflow when depd instance is already constructed.

Ah, yes. Is it possible you can open an new issue and we can continue the discussion there? You can paste the exact error with it's stack that is thrown when your code hits that line you linked to to get started.

charandas commented 6 years ago

Yes, will do.