-
-
Notifications
You must be signed in to change notification settings - Fork 697
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
inspect should skip properties that begin with $ #1109
Comments
For those experiencing this problem, you may be interested to know that I was able to work around it using intercept-require. const interceptRequire = require('intercept-require');
const uninterceptRequire = interceptRequire((mExport, info) => {
if (info.moduleId == './getEnumerableProperties' &&
info.callingFile.endsWith('/node_modules/chai/lib/chai/utils/inspect.js')) {
mExport = (obj) => {
const props = []
for (let name in obj) {
if (!name.startsWith('$')) props.push(name)
}
return props
}
}
return mExport
})
const chai = require('chai')
uninterceptRequire() |
I've been working on a new version of the inspection utility which handles all of these things generically - so for example it can detect circular references and not hang. I hope to get something released soon. |
I had to make an update this intercept logic to work across platforms: const interceptRequire = require('intercept-require')
const patchChai = (_, info) => {
if (
info.moduleId.endsWith('/getEnumerableProperties') &&
(info.absPath.replace(/\\/g, '/') || `/node_modules/${info.moduleId}`).endsWith(
'/node_modules/chai/lib/chai/utils/getEnumerableProperties.js'
)
) {
return (obj) => {
const props = []
for (let prop in obj) {
if (!prop.startsWith('$')) props.push(prop)
}
return props
}
}
}
const uninterceptRequire = interceptRequire(patchChai)
const chai = require('chai')
uninterceptRequire() |
imo, skipping objects starting with one can avoid traversing thru circular objects by keeping a list of "seen" objects and performing a strict equality check. this is what mocha does when "stringifying" an object. does that not apply to this case? |
It does and is fixed in the inspection engine rewrite. I doubt we want to really block arbitrary string suffixes. |
Before I say anything else, I want to revisit the big picture. chai should not hang the node process and eat up 100% of the CPU. (When that process pegging occurs on a cloud server I'm paying for by the hour, this problem gets expensive very fast). If that's been fixed (in the rewrite), then the main concern is addressed. I recognize $ is quite particular. Still, the reality is that Opal exists and ends up on the load path of many projects for one reason or another and we have to deal with it. I'm not saying let's ignore all kinds of crazy prefixes. I'm saying let's deal with a problem we have this ecosystem that's real. Perhaps we could consider adding an optional property exclude pattern option to handle cases like this. I could live with that. It's far better than having to hack the internals of chai, which is extremely fragile. |
IMO (I'm not a maintainer of Chai), an option to do such a thing as "exclude certain properties from object diffs based on a pattern" is also quite particular. Are these viable workarounds?
But, honestly, if Opal chooses to do stuff like modifying |
Yep. Absolutely. There's two problems here. Firstly the inspection engine hangs. This will be fixed in the next few months as we introduce
This is a valid concern, but Opal is one library out of N. Making concessions for individual libraries is not something we're interested in doing, as its a slippery slope. We could also block the TL;DR. Sorry, it sucks that this is the case right now. We're working hard to fix it. Rest assured it will be fixed - just not in the exact way that Opal declares it's circular properties. |
I'm going to close this, as we're clearing house on any issues as we're about to do a bunch of refactoring work in Chai 5 - which will end up implictly fixing this issue. |
That's fine. In fact, the discussion prompted a change in Opal itself. Opal is now going to define properties using the defineProperty mechanism (with enumerable set to false), which will exclude them from being picked up by for...in. See opal/opal@34f89df |
Opal (Ruby to JavaScript transpiler) adds special properties to all objects which introduce a circular graph. For example:
When inspect encounters one of these objects, it enters an infinite recursion and hangs.
inspect should skip properties on an object that begin with
$
. If not the default, it should provide a configuration switch to activate this filtering.You could debate that Opal should not be adding properties that are accessible via
(for name in obj)
. The problem is, the Opal creators don't agree, and that leaves us all that use it with test suites that hang on failures.Here's the proposed change to getEnumerableProperties.js.
The text was updated successfully, but these errors were encountered: