Skip to content
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

Depends on a global "document" #343

Closed
matthewp opened this issue Aug 14, 2015 · 7 comments
Closed

Depends on a global "document" #343

matthewp opened this issue Aug 14, 2015 · 7 comments

Comments

@matthewp
Copy link

jQuery has the ability to return a function when a document doesn't exist so that you can provide your own document (such as in Node). However Sizzle makes this functionality not work, because it does expect a global document here.

matthewp added a commit to matthewp/sizzle that referenced this issue Aug 14, 2015
Sizzle creates a local variable `document`, however it remains unset at
startup so assertions will throw if there isn't a global `document`,
such as in Node. Defining the initial value of the document `var` fixes
this.

Closes jquery#343
matthewp added a commit to matthewp/sizzle that referenced this issue Aug 14, 2015
Sizzle creates a local variable `document`, however it remains unset at
startup so assertions will throw if there isn't a global `document`,
such as in Node. Defining the initial value of the document `var` fixes
this.

Closes jquery#343
@gibson042
Copy link
Member

That's not the global document; see setDocument.

@matthewp
Copy link
Author

I understand closing a PR as incorrect but why would you close an issue without getting clarity on the problem?

I posted this in the (closed) PR issue but jQuery allows itself to be ran in an environment without a document by passing in a window object that has a document. So you can do this:

var makejQuery = require("jquery");

var jQuery = makejQuery(myFakeWindow);

jQuery supports this. Sizzle does not, so it breaks. I apologize for the incorrect fix, any advise on what needs to be done to fix this would be appreciated.

@gibson042
Copy link
Member

My apologies; this looked like a non-issue. It still may be, but the above is insufficient to properly assess... what is myFakeWindow, what call throws an exception, and how is Sizzle responsible?

jQuery itself guarantees something very similar, and I just manually confirmed that basic Sizzle functionality remains operative in such an environment. Please provide a more complete test case demonstrating what you want to do and where it fails (a Node.js script will be just fine—sorry for suggesting the obviously inapplicable JS Bin), and I'll happily take a deeper look.

@gibson042 gibson042 reopened this Aug 15, 2015
@mgol
Copy link
Member

mgol commented Aug 15, 2015

Regardless of this particular issue, jQuery now doesn't assume the browser environment by declaring globals explictely in its .jshintrc instead of just using "browser": true. Such a change is not present in Sizzle's src/.jshintrc and it should.

@matthewp
Copy link
Author

My apologies; this looked like a non-issue. It still may be, but the above is insufficient to properly assess... what is myFakeWindow, what call throws an exception, and how is Sizzle responsible?

This is a difficult question to answer because what I am attempting to do is figure out how much of the DOM API Sizzle needs to load. I don't expect you to know the answer to this question. So to start off with I gave it this:

var makeFakeWindow = {
  document: {}
};

Again, I expect this to fail. I just don't expect it to fail for the reason of document being undefined. Document is not undefined.

This line is where the exception occurs.

@gibson042
Copy link
Member

Thanks; that clears things up. Let me preface the following by pointing out that Sizzle is a DOM library and is useless without an essentially complete implementation. In fact, we assume certain baseline aspects, which are not documented and are subject to change at any time. Failure to even load when those assumptions are violated is not a bug. Further, it looks like you're trying to load not Sizzle, but jQuery (normal builds of which include Sizzle). The distinction is relevant, which I'll get to shortly. But I'm going to unequivocally recommend that you use a DOM library like jsdom rather than trying to bare-minimum your own.

That said, the current minimum "window" required to load Sizzle without exceptions—invocation being another story, though—looks like this:

{
  document: {
    documentElement: {},
    nodeType: 9,
    createElement: function() { return {}; }
  }
}

...which is still not enough to load jQuery. For that, you'll need

{
  document: {
    documentElement: {},
    implementation: {
      createHTMLDocument: function () {
        return {
          body: {
            childNodes: []
          }
        };
      }
    },
    nodeType: 9,
    addEventListener: function(){},
    createDocumentFragment: function () {
      return { appendChild: this.createElement };
    },
    createElement: function createElement() {
      var el = {};
      el.lastChild = el;
      el.appendChild = el.cloneNode = el.setAttribute = createElement;
      return el;
    }
  },
  location: {},
  addEventListener: function(){}
}

ALL of which, again, is sufficient only for loading and only as of this point in time.

@matthewp
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants