Bug Tracker

Ticket #13103 (closed feature: fixed)

Opened 22 months ago

Last modified 21 months ago

Avoid Boolean traps in the API

Reported by: ariya.hidayat@… Owned by:
Priority: low Milestone: 1.9
Component: effects Version: 1.9b1
Keywords: Cc:
Blocking: Blocked by:

Description

stop() function can lead to the following usage:

  $('foobar').stop(true, false);

The weird combination of two opposite Boolean values is a classic example of "Boolean trap".

See also:

Change History

comment:1 Changed 22 months ago by rwaldron

  • Milestone changed from None to 1.9

While I completely agree with you, there is nothing we can do without...

1) breaking backward compatibility

Or

2) creating an signature overload and deprecating the Boolean args.

I'd like to leave this for 1.9/2.0 discussion

comment:2 Changed 22 months ago by dmethvin

Totally agree on the boolean traps. Methods like .clone() have the same issue. But migrating out to a new API and actually removing the old one would take at least two years at this point, given that we're going to slow the pace after 1.9/2.0 arrives.

So, write the new documentation for these methods. Name? Description? Is the old .stop() method deprecated? Scheduled to be removed in 2.1? Or staying forever to allow old code to work since there is nothing functionally broken about the old API?

comment:3 Changed 22 months ago by rwaldron

If we go #2, we can slowly deprecate, but we trade for more code to maintain...

$(selector).clone({
  data: Boolean,
  events: Boolean
});

// Omitted options are assumed false, in this case "jumpToEnd" is false:
$(selector).clone({ data: true });

// Both options are assumed false
$(selector).clone();

// ------------------------------

$(this).stop({
  clearQueue: Boolean, 
  jumpToEnd: Boolean
});

// Omitted options are assumed false, in this case "jumpToEnd" is false:
$(this).stop({ clearQueue: true });

...Which is actually quite nice.

comment:4 Changed 22 months ago by dmethvin

It's worse though:  http://api.jquery.com/stop/

There's an optional queue parameter at the FRONT, so it's already doing hockey. I doubt many people use that signature with custom queues (it was just added in 1.7) so perhaps we could drop that outright.

comment:5 Changed 22 months ago by rwaldron

I don't think we need to drop it... it will just become one of the options. If the type param is an object re-assign the property values to the matching named args as needed and continue as done previously?

comment:6 Changed 22 months ago by timmywil

  • Priority changed from undecided to low
  • Status changed from new to open
  • Version changed from git to 1.9b1
  • Component changed from unfiled to effects

@rwaldron: +1

comment:7 Changed 22 months ago by dmethvin

If we want to land anything for 1.9 it should happen before the next beta, which means, um, today.

I'd like to avoid going from a "boolean trap" to a "misspelled names in anonymous objects" trap.

If .stop({ clearQueue: true }) is the same as .clearQueue().stop() (it seems to be) we should just advise people to use it. That costs us zero bytes.

For .stop({ jumpToEnd: true }) perhaps we could introduce a new API method that makes it clear what is happening, something like .finish() that implies we're completing the animation steps immediately and not really .stop()ping it dead in its tracks.

So this would be the mapping from old to new API:

  .stop(false, false)  ===>  .stop()
  .stop(true, false)   ===>  .clearQueue().stop()
  .stop(false, true)   ===>  .finish()
  .stop(true, true)    ===>  .clearQueue().finish()

All that needs to be done is adding .finish() and changing the docs to deprecate the boolean args and provide the recommended alternatives.

======

For .clone(withDataAndEvents, deepWithDataAndEvents) there needs to be a separate discussion. Even though it accepts two booleans I suspect damn near every call just uses 0 or 1 and I'd just prefer to leave it alone.

comment:8 Changed 22 months ago by jaubourg

The .clone() situation can be dealt with by accepting a string as the first argument.

.clone( true ) => with data and events
.clone("deep") => with data and events, deep

The logic inside is quite simple:

if ( withDataAndEvents ) {
    // deal with data and events
    if ( withDataAndEvents === "deep" ) {
        // do it recursively
    }
}

You can keep the old second param by changing the last condition to:

deepWithDataAndEvents || withDataAndEvents === "deep"

Pretty straight-forward. I dunno why we went for two separate arguments given how closely related they are.

comment:9 Changed 22 months ago by mikesherov

"magic string trap", "boolean trap", "misspelled property name in anonymous object trap" all sound like traps to me.

A boolean trap is simply "bad API design" and may lead to ambiguity as to purpose, but is straightforward in implementation. Magic strings make it harder to detect errors in userland, same goes for anonymous objects.

IMO, dmethvin's approach of creating new named API functions seems like the "right" "fix", in that it will error out on misspellings, and be clear in intention: .clone(), .cloneWithData(), .cloneDeepWithData() Of course, this ruins the jQuery aesthetic of short function names, but I think it's a clarity win.

I'd say all boolean parameters should be considered for re-evaluation, but the one that comes to mind for me is outerWidth(true). The dimension functions should be called: contentBoxWidth(), paddingBoxWidth(), borderBoxWidth(), and marginBoxWidth()... no booleans.

comment:10 follow-up: ↓ 11 Changed 22 months ago by dmethvin

@jaubourg, .clone(true) is the same as .clone(true, true) because the deep arg is the same as the first arg unless it's explicitly specified. So the only time it comes into play is if you explicitly say .clone(false, true) and I can't imagine a reason why anyone would need to specifically do that.

So although a .cloneWithDataAndEvents() might make sense, to capture the current behavior we'd need something like cloneWithDataAndEventsExceptForTheRoot() which again I think is so rare that we just act like it isn't there.

comment:11 in reply to: ↑ 10 Changed 22 months ago by jaubourg

Replying to dmethvin:

@jaubourg, .clone(true) is the same as .clone(true, true) because the deep arg is the same as the first arg unless it's explicitly specified. So the only time it comes into play is if you explicitly say .clone(false, true) and I can't imagine a reason why anyone would need to specifically do that.

It's even worse than what I (wrongly) assumed :/

So although a .cloneWithDataAndEvents() might make sense, to capture the current behavior we'd need something like cloneWithDataAndEventsExceptForTheRoot() which again I think is so rare that we just act like it isn't there.

+1

@mikesherov, seems to me you see traps everywhere... how about mispelled or badly named API functions? It's a trap!

There is no silver bullet. Booleans, strings, separate functions, it all boils down to the actual use case.

The boolean trap comes into play when you have several boolean arguments next to one another, not when you have a single boolean argument. With a single argument, the question is about ambiguity. This is the problem with the outerWidth method: true/false don't have any obvious meaning in this context.

Beside, that's why I actually love "magical" strings. They work perfectly to create $.Callbacks for instance as they should in any situation where you need to specify several "flags". I always find them to be very easy to read and make sense of given how inherently explicit they are.

comment:12 Changed 22 months ago by mikesherov

Obligatory Admiral Akbar reference:  http://www.youtube.com/watch?v=piVnArp9ZE0

comment:13 Changed 22 months ago by timmywil

My first thought on the .finish() method is that it would complete all animations, or at least have an option to complete all animations. It seems more useful to make sure all animations on the element are done before I move on to something else.

comment:14 Changed 22 months ago by dmethvin

I was thinking we might add a .finishAll() method for that reason but then again if you wanted to finish *only* the current step you could .clearQueue().finish() right? So it seems the combo of those two APIs cover all the cases without need for a boolean option to .finish().

The interesting thing about that is that there is no easy way to do the equivalent of finishing all animation steps today and I've wanted it a couple of times. So I'd go for that. Other thoughts?

comment:15 Changed 21 months ago by dmethvin

  • Status changed from open to closed
  • Resolution set to fixed

We landed .finish() for 1.9.0 and the only todo is for us to talk people out of using the .stop() signature.  https://github.com/jquery/api.jquery.com/issues/236

Note: See TracTickets for help on using tickets.