Chai: inspect should skip properties that begin with $

[mojavelinux]

Opal (Ruby to JavaScript transpiler) adds special properties to all objects which introduce a circular graph. For example:

require('opal-runtime')

const a = []
console.log(a['$class']['$class']['$class']['$class']['$class']['$class'])

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.

module.exports = function getEnumerableProperties(object) {
  var result = [];
  for (var name in object) {
    if (!name.startsWith('$')) result.push(name);
  }
  return result;
};

[mojavelinux]

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()

[keithamus]

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.

[mojavelinux]

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()

[boneskull]

imo, skipping objects starting with $ is quite particular and better served by a custom plugin than in a core codebase.

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?

[keithamus]

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.

[mojavelinux]

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.

[boneskull]

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?

• remove the properties before asking Chai to evaluate them
• write a plugin for Chai to do such a thing before making assertions

But, honestly, if Opal chooses to do stuff like modifying Object.prototype, the responsibility for mitigating the effects of such a mistake shouldn't fall upon projects like this one.

[keithamus]

chai should not hang the node process and eat up 100% of the CPU

Yep. Absolutely. There's two problems here. Firstly the inspection engine hangs. This will be fixed in the next few months as we introduce loupe as a new inspection engine, which fixes circular refs. The next problem is why the inspection engine is ever run for passing tests. This is addressed in #585 which is a much longer architectural change.

I'm saying let's deal with a problem we have this ecosystem that's real.

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 $$typeof$$ magic strings that some libraries like React have, but its not an interest we have. Again though, the new inspection engine will enable plugins to interface with these paths to allow for this kind of behavior.

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.

[keithamus]

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.

[mojavelinux]

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 https://github.com/opal/opal/commit/34f89df1d700be5143b9dbcc9886202fd2bfd3ca