[Discussion] Consider using stricter brand checks than just Symbol.toStringTag?

[jcbhmr]

trafficstars

Right now if you use something like a math library that defines a Symbol class for math operations and do isSymbol() on it, there is a serious non-zero chance that it will return true! 😱

FancyMath.Symbol = class Symbol {
  constructor(expression: string, id: string) {
    // ...
  }

  [Symbol.toStringTag] = "Symbol"
}

const x = new FancyMath.Symbol("x + y", "x")
console.log(isSymbol(x))
//=> true

This happens because isSymbol() relies on Symbol.toStringTag via Object#toString() to "sniff" the type. There are other more robust ways for this particular type, though, and I think they should be used.

Example:

// Returns TRUE for Symbol() primitives
// Returns TRUE for Object(Symbol()) boxed primitives
// Returns TRUE for Object.create(Symbol.prototype)
// Returns TRUE for classes that extend Symbol
// Returns FALSE for the FancyMath.Symbol
// Returns TRUE for FancyMath.Symbol if it's from another realm
function isSymbol(x) {
  if (typeof x === "symbol") {
    return true
  } else if (x && typeof x === "object") {
    if (x instanceof Object) {
      // Same realm object
      return x instanceof Symbol
    } else if (!Object.getPrototypeOf(x)) {
      // Null-prototype object
      return false
    } else {
      // Object that is probably cross-realm
      return Object.prototype.toString.call(x).slice(8, -1) === "Symbol"
    }
  } else {
    return false
  }
}

Or, you could get fancier and do a branch check on the .description getter! If it throws, it's not a symbol.

// Returns TRUE for Symbol() primitives
// Returns TRUE for Object(Symbol()) boxed primitives
// Returns FALSE for Object.create(Symbol.prototype)
// Returns TRUE for classes that extend Symbol
// Returns FALSE for the FancyMath.Symbol
// Returns idk for FancyMath.Symbol if it's from another realm
function isSymbol(x) {
  try {
    Object.getOwnPropertyDescriptor(Symbol.prototype, "description").get.call(x)
  } catch {
    return false
  }
  return true
}

[jcbhmr]

My vote is to follow the existing known conventions and try to mirror the Node.js node:util isSymbol() and friends APIs and how they detect things. https://nodejs.org/api/util.html

In this case, for isSymbol(), that means typeof x === "symbol" instead of the current version that uses Symbol.toStringTag

[jcbhmr]

If you want, you can just copy-paste code from https://github.com/nodefill/util/tree/main/src/types