Handle view.query() being invoked when view.el is undefined

[nirrek]

Currently, if view.el is undefined, a call to view.query() will fail with a Uncaught TypeError: Illegal invocation inside the matches-selector package. It would be nicer to have a check in the query method that handles this condition.

query: function (selector) {
        if (!selector) return this.el;
        if (typeof selector === 'string') {
            if (matches(this.el, selector)) return this.el;  // this causes TypeError when this.el is undefined
            return this.el.querySelector(selector) || undefined;
        }
        return selector;
    }

You have a guard for this condition in the queryAll method, just not in the query method.

queryAll: function (selector) {
        var res = [];
        if (!this.el) return res;  // guards against condition where this.el === undefined
        if (selector === '') return [this.el];
        if (matches(this.el, selector)) res.push(this.el);
        return res.concat(Array.prototype.slice.call(this.el.querySelectorAll(selector)));
    },

[wraithgar]

In cases like this we've tended towards a more helpful exception. If you query against a nonexistent el it should still probably throw, but it should throw a specific error like 'this.el not defined'

ETA: This is just my opinion, adding discussion label.

[wraithgar]

In cases like this we've tended towards a more helpful exception. If you query against a nonexistent el it should still probably throw, but it should throw a specific error like 'this.el not defined'

ETA: This is just my opinion, adding discussion label.

[kamilogorek]

Agree with @wraithgar Querying is only possible on rendered elements or something similar.

[kamilogorek]

Agree with @wraithgar Querying is only possible on rendered elements or something similar.

[wraithgar]

Just to be clear, the current options are: add the guard to query that already exists in queryAll, or make both throw a descriptive exception if this.el is not defined

[wraithgar]

Just to be clear, the current options are: add the guard to query that already exists in queryAll, or make both throw a descriptive exception if this.el is not defined

[nirrek]

I think having a useful exception thrown is preferable to simply returning an empty array or undefined.

Empty array / undefined should mean that DOM traversal was correctly performed, but there were no matches found.

When el is undefined, you never actually perform a search at all. This will never be the intention of the user, so they should be notified that they are doing something incorrectly.

Possible exception message:
Query cannot be performed as this.el is not defined. Ensure that the view has been rendered.

[nirrek]

I think having a useful exception thrown is preferable to simply returning an empty array or undefined.

Empty array / undefined should mean that DOM traversal was correctly performed, but there were no matches found.

When el is undefined, you never actually perform a search at all. This will never be the intention of the user, so they should be notified that they are doing something incorrectly.

Possible exception message:
Query cannot be performed as this.el is not defined. Ensure that the view has been rendered.

[wraithgar]

I like that. This would still be a breaking change as anyone currently relying on the guard statement would now be getting an exception thrown.

[wraithgar]

I like that. This would still be a breaking change as anyone currently relying on the guard statement would now be getting an exception thrown.

[pgilad]

^ Added a PR for this. I agree error should be thrown as to be verbose.
Notice:

1. Change queryAll to first throw if this.el doesn't exist, then possibly return [this.el] if !selector.
2. Breaking change for queryAll
3. Test?

[pgilad]

^ Added a PR for this. I agree error should be thrown as to be verbose.
Notice:

1. Change queryAll to first throw if this.el doesn't exist, then possibly return [this.el] if !selector.
2. Breaking change for queryAll
3. Test?