ljharb / js-traverse

MIT License
45 stars 8 forks source link

Does not work with builtin objects with internal slots (URL, Map, Set, etc) #10

Open danilofuchs opened 3 months ago

danilofuchs commented 3 months ago

For instance:

    traverse({ url: new URL("https://example.com") }).map(function () {
      if (this.node instanceof URL) {
        this.update(this.node.toString());
      }
    });
TypeError: Receiver must be an instance of class URL
 ❯ Object.<anonymous> src/log-sanitizer.spec.ts:57:31
     55|     traverse({ url: new URL("https://example.com") }).map(function () {
     56|       if (this.node instanceof URL) {
     57|         this.update(this.node.toString());
       |                               ^
     58|       }
     59|     });
 ❯ walker node_modules/traverse/index.js:169:16
 ❯ node_modules/traverse/index.js:190:17
 ❯ forEach node_modules/traverse/index.js:19:30
 ❯ walker node_modules/traverse/index.js:185:4
 ❯ walk node_modules/traverse/index.js:208:3
 ❯ Traverse.map node_modules/traverse/index.js:251:9
ljharb commented 3 months ago

hm, interesting - we have some copying that would lose the internal URL slots, so this isn't too surprising.

In general, this library is built for plain objects - if you do traverse({ x: new Map() }).map(function () { return this.node.size; }) you'll get a similar error.

In general, instanceof is simply not a reliable tool in the language and should be avoided, both because it doesn't check internal slots and because Symbol.hasInstance can be broken or forged.

In this case, to avoid the crash, you'd need to be guarding with a proper isURL predicate (which doesn't exist on npm, so i guess i'll have to make one :-p ) - but that still wouldn't solve your underlying issue which is that you want to be able to get at the native toString behavior on a URL.

I think the general problem of "how do i get to the original, uncopied value" is worth addressing and considering a flaw, if not a bug.

ljharb commented 2 months ago

tbh I'm not sure how to fix this.

One option is to laboriously list all language, node, and web builtins, and check for these as a "leaf".

A simpler option would be to accept a callback option, like isLeaf, so you can decide if the value should be traversed further, or not.