Things are used from global without properly qualifying them

[domenic]

Since jsdom isn't in the browser, things like Element aren't global there, so things like https://github.com/dperini/nwsapi/blob/master/src/nwsapi.js#L1334-L1337 don't work there (and cause crashes)

I wonder if install/uninstall could be removed from the jsdom-facing version entirely.

[domenic]

https://github.com/dperini/nwsapi/blob/816c909a69f1c9db0700a73df1f8b47598acc554/src/nwsapi.js#L452 also has this problem

[domenic]

And, as alluded to in #1, https://github.com/dperini/nwsapi/blob/816c909a69f1c9db0700a73df1f8b47598acc554/src/nwsapi.js#L477

[dperini]

@domenic,

I wonder if install/uninstall could be removed from the jsdom-facing version entirely.

Sure I could remove that completely ... however I am trying to have only one version for both browsers and 'jsdom' and for browsers I am trying to give an easy path to users to replace the native QSA API with 'nwsapi'. Given that the 'install/uninstall' methods are not invoked/used in the code anywhere my suggestion is to slightly change my code to never be executed by 'jsdom' nor use 'Element'' interface but still leave it in. This means moving the placeholder assignment inside the methods and just leave the globals out.

If you agree with this extra payload of a few hundred bytes I may be able to keep both kind of users happy. I will commit a change in the code for you to review in a few hours.

Also if you see browsers implementation errors in install/uninstall maybe you can help improve those methods.

Do you find this to be an acceptable compromise ?

[dperini]

Issues #1 and #2 were fixed.

Still pending a resolution about install/uninstall methods in the comments of issue #2.

[domenic]

Sure, it's fine to keep them. It does require jsdom pass dummy versions of Element, Document etc. to nwsapi (objects with a single property, prototype, which is an empty object). This is kind of a silly API for a Node module to have, where you always have to give it dummy objects before it can work. But we can use it.

[domenic]

Here is how we use nwsapi in my local branch, so you can see what I mean:

const DOMException = require("domexception");

// TODO: remove these workarounds depending on how https://github.com/dperini/nwsapi/issues/2 goes.
const dummy = { prototype: {} };
function dummyXMLDocument() { } // TODO maybe this should be better. Probably we do want different XML selector behavior.

function createNWSAPI(document) {
  const nwsapiInstance = nwsapi({
    document,
    DOMException,
    Element: dummy,
    Document: dummy,
    XMLDocument: dummyXMLDocument
  });
  nwsapiInstance.configure({ VERBOSITY: false });

  return nwsapiInstance;
}

This is using a hacked copy of nwmatcher where I just added global. before everything.

[domenic]

Oh, I didn't see that you changed this in https://github.com/dperini/nwsapi/commit/6bc9e0692298f8bc301a9ac817c8c4f0abb36d14. That fix looks pretty good and I've confirmed it works in jsdom. So I'll close this :)