dougwilson / nodejs-depd

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

Call site calculation fails when importing an esm'ed package that imports sequelize internally #28

Closed charandas closed 6 years ago

charandas commented 6 years ago

@dougwilson As promised in #27 here is the repro of the issue I am having. Use the with-esm branch to see the issue.

The without-esm branch is fine with importing sequelize, so maybe not worth seeing.

I tested with couple Node versions, v8.9.0 and v10.7.0, so I doubt the version matters there.

charandas commented 6 years ago

Just added a readme to the repo. Let me know if anything about the setup is unclear.

dougwilson commented 6 years ago

Thanks, I will take a look soon. Can you please post the exact error with it's full stack trace that is being thrown?

charandas commented 6 years ago

I remember seeing longer stacktraces couple weeks ago, which go to common sequelize depd file, but can no longer create a long stacktrace. Here is what you will see with the repo:

    TypeError: callSite.getFileName is not a function

      at callSiteLocation (packages/some-esm-package/node_modules/depd/index.js:252:23)
charandas commented 6 years ago

Also, the same issue happens whether you use ts-jest or jest. For using latter, one has to remove the transform portion from package.json ("^.+\\.(ts|tsx)$": "ts-jest"), as well as change the spec.ts file to spec.js (stripping it of ES6 imports).

I created the repro with ts-jest since that is my current use-case.

dougwilson commented 6 years ago

So thanks for the repo. This is a Node.js bug: https://github.com/nodejs/node/issues/21270 and since esm lib runs code inside a vm context it triggers the bug.

dougwilson commented 6 years ago

Sorry, didn't mean to click comment + close.

dougwilson commented 6 years ago

I'm trying to see if it's possible there is a user-land solution but I cannot find one, and it may not be possible to fix without Node.js core involvement. I may be able to detect this brokenness at runtime and load the browser version (which is basically a complete no-op) instead. This would make your code work, though you'd get no deprecation warnings from any module in return.

dougwilson commented 6 years ago

Unfortunately since it seems like the esm lib is minified on install and does not provide a source map, it's pretty much impossible to debug. To move forward and get a test I need to figure out how to invoke vm in the same way esm is doing so I can add a test to the test suite (and also understand it better to know if there is actually a work-around or not).

charandas commented 6 years ago

May wanna involve @jdalton for a quick answer. Hope this doesn't count as spam.

jdalton commented 6 years ago

Thanks for the ping and the repro. I'll dig in soon.

Update:

It's related to this issue (V8 patch here).

Simple repro with nvm to version switch.

Try Node 6.x:

s=new vm.Script('var o={};Error.prepareStackTrace=(o,s)=>s;Error.captureStackTrace(o);o.stack')
s.runInNewContext(vm.createContext(global)) // an array [CallSite {},...]

In Node 8+:

s=new vm.Script('var o={};Error.prepareStackTrace=(o,s)=>s;Error.captureStackTrace(o);o.stack')
s.runInNewContext(vm.createContext(global)) // a string "'Error\n    at ev..."

The reason esm is running into this is we try to run in a jest specific context which causes the stack to not be properly prepared by depd resulting in a string instead of the expected array.

I'll see what we can do from our end.

Update:

Patch: https://github.com/standard-things/esm/commit/c464c35b441a8cdccd582b8e6a0f4483e16cc84d Tests: https://github.com/standard-things/esm/commit/8216c8fb81854390260ce151dfa15382f13c2fac

dougwilson commented 6 years ago

@jdalton thanks for taking a look and even making a patch so quickly! I see now that we both thought it was the same Node.js but, but apparently it was slightly different -- since the .stack is evaluated in the same v8 context the error object was made, it seemed off, but looking at your patch, I see what was happening now: the Error global that was inside the context actually belonged to the parent context, so then this module accessed .stack the v8 code didn't see the prepareStackTrace set on the Error that belonged to the context.

I have also figured out how, at least in this case, I can get a reference to the Error object I was expecting: require.constructor('return Error')() . I'm not sure if that will be fragile or not, though. I think your change in esm was along the right lines, though hopefully that doesn't just cause different breakage in some module 😆.

Thanks @charandas for reporting this! I believe that this is unlikely to have been the same cause as the other issue you were posted in.

charandas commented 6 years ago

Thanks to you both.

charandas commented 6 years ago

@jdalton could that patched package be released soon?

jdalton commented 6 years ago

@charandas Yes, it'll be released this by this afternoon.

Update:

v3.0.77 is released :tada: