Using a malformed selector throws a dom exception 12. #619

Closed
dristic opened this Issue Oct 18, 2012 · 17 comments

Comments

Projects
None yet
6 participants
@dristic

dristic commented Oct 18, 2012

When you use a malformed selector in zepto such as:

$("myCrazySelctor (this throws an exception)")

It will throw a dom exception 12 because it is passing this directly to the query selector function. Can we put a try/catch around the zepto.qsa function so that if you pass a bad selector it does not break the entire page?

@alanhogan

This comment has been minimized.

Show comment
Hide comment
@alanhogan

alanhogan Oct 23, 2012

That’d be so good. Maybe it could suggest compiling with experimental selector support?

That’d be so good. Maybe it could suggest compiling with experimental selector support?

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Oct 24, 2012

Collaborator

The selector module includes support for some non-standard jQuery selectors.

However if you use invalid selectors, I think you should be getting DOM errors. This way, you get to know that you're doing something wrong.

You can always wrap your find statements in a try/catch block.

Collaborator

mislav commented Oct 24, 2012

The selector module includes support for some non-standard jQuery selectors.

However if you use invalid selectors, I think you should be getting DOM errors. This way, you get to know that you're doing something wrong.

You can always wrap your find statements in a try/catch block.

@mislav mislav closed this Oct 24, 2012

@dristic

This comment has been minimized.

Show comment
Hide comment
@dristic

dristic Oct 24, 2012

The issue I have with it is that it is unstable. Passing one malformed selector crashes the whole page.

If you need a fix for this just set $ to a function that wraps $ in a try/catch. That is what I used.

dristic commented Oct 24, 2012

The issue I have with it is that it is unstable. Passing one malformed selector crashes the whole page.

If you need a fix for this just set $ to a function that wraps $ in a try/catch. That is what I used.

@alanhogan

This comment has been minimized.

Show comment
Hide comment
@alanhogan

alanhogan Oct 24, 2012

What would you do in the catch? You can’t continue if you didn’t select the elements your code assumes you did.

What would you do in the catch? You can’t continue if you didn’t select the elements your code assumes you did.

@dristic

This comment has been minimized.

Show comment
Hide comment
@dristic

dristic Oct 24, 2012

You can return a $('thisWillSelectNothing') which will return a selection of nothing. This way your code will still run but not effect any elements.

dristic commented Oct 24, 2012

You can return a $('thisWillSelectNothing') which will return a selection of nothing. This way your code will still run but not effect any elements.

@bnickel

This comment has been minimized.

Show comment
Hide comment
@bnickel

bnickel Oct 24, 2012

Contributor

This behavior does reflect what the behavior of jQuery but is a little more extreme. jQuery will fail $('test:'), $(':test') and $('@'). Interestingly $(':test') and $('*:test') throw exceptions but $('test:test') returns an empty jQuery object. This is despite their API docs saying that unmatched selectors will always return an empty jQuery object.

Given jQuery's behavior, I don't feel this is mandatory to implement and could actually hinder debugging. I would recommend a small addition to the API docs to say that an exception is a possibility for malformed selectors.

As a general comment, this behavior should never surface in production code if you're following a best practice of never passing dynamic values into $. If you have to composite strings, you should consider splitting into separate find or get operations.

Contributor

bnickel commented Oct 24, 2012

This behavior does reflect what the behavior of jQuery but is a little more extreme. jQuery will fail $('test:'), $(':test') and $('@'). Interestingly $(':test') and $('*:test') throw exceptions but $('test:test') returns an empty jQuery object. This is despite their API docs saying that unmatched selectors will always return an empty jQuery object.

Given jQuery's behavior, I don't feel this is mandatory to implement and could actually hinder debugging. I would recommend a small addition to the API docs to say that an exception is a possibility for malformed selectors.

As a general comment, this behavior should never surface in production code if you're following a best practice of never passing dynamic values into $. If you have to composite strings, you should consider splitting into separate find or get operations.

@alanhogan

This comment has been minimized.

Show comment
Hide comment
@alanhogan

alanhogan Oct 24, 2012

Maybe there should be a third build of Zepto.

  1. Minified
  2. Development
  3. Transitioning from jQuery

In that third build, we could catch these errors and give a friendly error message, and jQuery methods we could support could all be stubbed out to similarly return a message that it is not actually part of that Zepto build.

Interesting enough to make a separate issue?

Maybe there should be a third build of Zepto.

  1. Minified
  2. Development
  3. Transitioning from jQuery

In that third build, we could catch these errors and give a friendly error message, and jQuery methods we could support could all be stubbed out to similarly return a message that it is not actually part of that Zepto build.

Interesting enough to make a separate issue?

@dristic

This comment has been minimized.

Show comment
Hide comment
@dristic

dristic Oct 25, 2012

I don't think the work it would take to create a build just for transitioning from jQuery is worth it. If none of the methods work then it should just error out and you can fix your errors.

Unfortunately I cannot get around passing dynamic values through the selector in my project. I will just have to live with wrapping the selector function.

dristic commented Oct 25, 2012

I don't think the work it would take to create a build just for transitioning from jQuery is worth it. If none of the methods work then it should just error out and you can fix your errors.

Unfortunately I cannot get around passing dynamic values through the selector in my project. I will just have to live with wrapping the selector function.

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Oct 25, 2012

Collaborator

You can return a $('thisWillSelectNothing')

So you want to silently return an empty collection on invalid CSS
selectors, without any warning as to what you did wrong, and later let
yourself wonder why later operations don't have any effect?

It's not a good idea to be "jQuery compatible" in this regard, because
jQuery wasn't standards-compatible to begin with. jQuery selector engine
supports all sort of weird selectors that browsers don't support (some of
which I hacked together in "selector" module). If you transition to Zepto,
you'd want to get informed of selectors that you previously could use and
that don't work anymore.

You'll want to catch it sooner than later. Hence exceptions

Collaborator

mislav commented Oct 25, 2012

You can return a $('thisWillSelectNothing')

So you want to silently return an empty collection on invalid CSS
selectors, without any warning as to what you did wrong, and later let
yourself wonder why later operations don't have any effect?

It's not a good idea to be "jQuery compatible" in this regard, because
jQuery wasn't standards-compatible to begin with. jQuery selector engine
supports all sort of weird selectors that browsers don't support (some of
which I hacked together in "selector" module). If you transition to Zepto,
you'd want to get informed of selectors that you previously could use and
that don't work anymore.

You'll want to catch it sooner than later. Hence exceptions

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Oct 25, 2012

@mislav Maybe some kind of "transitional" module would be useful, which would just console.log a warning about this, but doesn't throw anything.

@mislav Maybe some kind of "transitional" module would be useful, which would just console.log a warning about this, but doesn't throw anything.

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Oct 25, 2012

Collaborator

Sure, here's a simple transitional module for you:

var originalQsa = Zepto.zepto.qsa

Zepto.zepto.qsa = function(el, sel) {
  try {
    return originalQsa.apply(this, arguments)
  } catch(e) {
    console.error("invalid CSS selector: ", sel)
    return []
  }
}
Collaborator

mislav commented Oct 25, 2012

Sure, here's a simple transitional module for you:

var originalQsa = Zepto.zepto.qsa

Zepto.zepto.qsa = function(el, sel) {
  try {
    return originalQsa.apply(this, arguments)
  } catch(e) {
    console.error("invalid CSS selector: ", sel)
    return []
  }
}
@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Oct 25, 2012

@mislav Well, yeah, that works, but i meant generally a transitional module that emulates jQuery as close as possible and just warns. Generally, not just about invalid selectors.

@mislav Well, yeah, that works, but i meant generally a transitional module that emulates jQuery as close as possible and just warns. Generally, not just about invalid selectors.

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Oct 25, 2012

Collaborator

That's an interesting idea! You're welcome to write it as a plugin that we
might include as an optional module.

Or, if you don't have time to write it, just list the specific things you'd
want to be warned about.

Collaborator

mislav commented Oct 25, 2012

That's an interesting idea! You're welcome to write it as a plugin that we
might include as an optional module.

Or, if you don't have time to write it, just list the specific things you'd
want to be warned about.

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Oct 25, 2012

Well, it would have made my move from jQuery easier if Zepto raised a more informative error message than DOMException-something. Something like InvalidZeptoSelector would be nice. The current message might make perfect sense for you if you know about the methods Zepto uses, but to me it looked like there was an error inside Zepto or even the browser's rendering engine.

Other than that and #604, i can't complain (or at least remember) about anything that made it hard/demotivating for me.

Well, it would have made my move from jQuery easier if Zepto raised a more informative error message than DOMException-something. Something like InvalidZeptoSelector would be nice. The current message might make perfect sense for you if you know about the methods Zepto uses, but to me it looked like there was an error inside Zepto or even the browser's rendering engine.

Other than that and #604, i can't complain (or at least remember) about anything that made it hard/demotivating for me.

@alanhogan

This comment has been minimized.

Show comment
Hide comment
@alanhogan

alanhogan Oct 25, 2012

On Oct 25, 2012, at 10:45 AM, Markus Unterwaditzer notifications@github.com wrote:

Well, it would have made my move from jQuery easier if Zepto raised a more informative error message than DOMException-something.

Exactly agreed :)

On Oct 25, 2012, at 10:45 AM, Markus Unterwaditzer notifications@github.com wrote:

Well, it would have made my move from jQuery easier if Zepto raised a more informative error message than DOMException-something.

Exactly agreed :)

@shalecraig

This comment has been minimized.

Show comment
Hide comment
@shalecraig

shalecraig Oct 29, 2012

Contributor

I noticed that if Zepto.js is compiled using the optional selector.js module (by running rake concat[:selector] dist), we get a built-in try/catch, but it still throws an error to the caller.

It's unreasonable to code like this (below), and the alternative of writing every selector inside a try/catch is also not good.

try {
  var x = $('foo1'),
  y = $('foo2');
  // etc.
} catch (e) {
  // oh well.
}

By us not incorporating try/catches inside Zepto, we allow it to be smaller, but the code that uses Zepto.js becomes larger.

For Zepto to be a mainstream library, it needs a predictable and safe API. I don't think the correct behaviour is throwing an error in production, but it is super handy while developing.

Akin to @alanhogan 's suggestion, we might want different builds of Zepto. We don't want to offer a "moving from jQuery" build, as it's not the goal of Zepto. We might want the ability to generate "development" and "production" versions of the code, akin to "min" and non-minified versions of Zepto.

My suggestion is as follows:

As part of the build process, we build four Zepto.js files, one for each permutation of minified (or non-minified), and production (or development) code.

This allows developers to debug using meaningful errors (fail early and fail fast), and lets Zepto have the production-ready consistency required for a decent library.

How could we possibly build & maintain this?

Uglify.js allows us to pass in a parameter (-d, for those who care) that defines global constants for the compiler's pre-processor to evaluate.

We would write code like this:

Zepto.foo = function(arg1, arg2) {
  var bar = function() {
    // Sketchy code.
  };
  try {
    return bar.call(this, arg1, arg2);
  } catch (e) {
    if (DEVELOPMENT) {
      throw e;
    } else {
      console.err(e);
    }
    return defaultValue;
  }
};

What are possible downsides?

  • Uglify.js might not inline the function call, so code might end up looking like this:
Zepto.foo = function(arg1, arg2) {
  var bar = function(arg1, arg2) {
    // sketchy code
  }
  try
    return bar.call(this, arg1, arg2)
  catch (e) {
    console.err(e)
    return defaultValue
  }
}

[sidenote: the closure compiler allows us to do this for sure]

I understand that we don't support bad code, but we should be dependable in production.

But that's just my 2¢.

Contributor

shalecraig commented Oct 29, 2012

I noticed that if Zepto.js is compiled using the optional selector.js module (by running rake concat[:selector] dist), we get a built-in try/catch, but it still throws an error to the caller.

It's unreasonable to code like this (below), and the alternative of writing every selector inside a try/catch is also not good.

try {
  var x = $('foo1'),
  y = $('foo2');
  // etc.
} catch (e) {
  // oh well.
}

By us not incorporating try/catches inside Zepto, we allow it to be smaller, but the code that uses Zepto.js becomes larger.

For Zepto to be a mainstream library, it needs a predictable and safe API. I don't think the correct behaviour is throwing an error in production, but it is super handy while developing.

Akin to @alanhogan 's suggestion, we might want different builds of Zepto. We don't want to offer a "moving from jQuery" build, as it's not the goal of Zepto. We might want the ability to generate "development" and "production" versions of the code, akin to "min" and non-minified versions of Zepto.

My suggestion is as follows:

As part of the build process, we build four Zepto.js files, one for each permutation of minified (or non-minified), and production (or development) code.

This allows developers to debug using meaningful errors (fail early and fail fast), and lets Zepto have the production-ready consistency required for a decent library.

How could we possibly build & maintain this?

Uglify.js allows us to pass in a parameter (-d, for those who care) that defines global constants for the compiler's pre-processor to evaluate.

We would write code like this:

Zepto.foo = function(arg1, arg2) {
  var bar = function() {
    // Sketchy code.
  };
  try {
    return bar.call(this, arg1, arg2);
  } catch (e) {
    if (DEVELOPMENT) {
      throw e;
    } else {
      console.err(e);
    }
    return defaultValue;
  }
};

What are possible downsides?

  • Uglify.js might not inline the function call, so code might end up looking like this:
Zepto.foo = function(arg1, arg2) {
  var bar = function(arg1, arg2) {
    // sketchy code
  }
  try
    return bar.call(this, arg1, arg2)
  catch (e) {
    console.err(e)
    return defaultValue
  }
}

[sidenote: the closure compiler allows us to do this for sure]

I understand that we don't support bad code, but we should be dependable in production.

But that's just my 2¢.

@bnickel

This comment has been minimized.

Show comment
Hide comment
@bnickel

bnickel Oct 29, 2012

Contributor

@shalecraig your example doesn't make sense. Neither $('foo1') or$('foo2') will cause an exception so there's no need for a try/catch. Something like $(':visible') will always throw an exception so there's also no need. The error is as accurate as the similar exceptions thrown by jQuery if not as precise.

The only case where a production exception would occur is in the case of $(x). This code is extremely dangerous and both Zepto and jQuery may throw exceptions. Really, they both should because the correct failure behavior is unknown to them. This isn't a jQuery comparability issue.

Contributor

bnickel commented Oct 29, 2012

@shalecraig your example doesn't make sense. Neither $('foo1') or$('foo2') will cause an exception so there's no need for a try/catch. Something like $(':visible') will always throw an exception so there's also no need. The error is as accurate as the similar exceptions thrown by jQuery if not as precise.

The only case where a production exception would occur is in the case of $(x). This code is extremely dangerous and both Zepto and jQuery may throw exceptions. Really, they both should because the correct failure behavior is unknown to them. This isn't a jQuery comparability issue.

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