I guess it doesn't like something about how we resolve promises with other promises. But I see nothing relevant in https://jquery.com/upgrade-guide/3.0/#deferred. I tried to debug this a bit, but I keep getting lost in jQuery's horrible Deferred code. Maybe @Krinkle wants to look into it.

I have no idea why, but changing the promise chaining to

.then( function ( opened ) {
    return opened;
} ).then( function ( closing ) {
    return closing;
} ).then( function ( data ) {
    return $.Deferred().resolve( !!( data && data.action === 'accept' ) );
} );

seems to work, although not completely correctly.

Also, I note that the whole chain isn't being called until the dialog closes, as opposed to in 1.11.3 where they're called at the appropriate times in the window's life cycle. This happens even if there's only the first 'then' in the chain and it doesn't return another promise.

I can't reproduce this on a MediaWiki page (using MediaWiki-Vagrant) as it just swallows the exception with no logging.

However, it's easy to reproduce on https://doc.wikimedia.org/oojs-ui/master/demos/.

OO.ui.confirm
- OO.ui.getWindowManager()
- windowManager.openWindow()
  - `typeof win === 'string'`
    - windowManager.getWindow().then
- windowManager.openWindow() // ↵ calls itself after lazy-init
  - opening.state() !== 'rejected'
    // this.closing, this.opened, this.opening are all null at this point
    - this.preparingToOpen = $.when( this.closing );
    // preparingToOpen immediately resolves with null,
    // its done() handler then assigns this.opening properly

OO.ui.confirm.then( function ( opened ) {
- opened // Object {action: "accept"}
- opened.then // fails, opened.then is undefined

Looks like it's somehow set to plain object {action: "accept"}, not a promise.

As indicated by @Anomie, if we ignore the opened parameter and return it as-is, then it will be internally seen as plain value and wrapped in a Promise (this is standard behaviour to allow promise handlers to synchronously filter a value in simple cases without manually needing to create and resolve a promise inline).

However in that case, data will not exist at the end of the chain.

Looking more closely, it looks like the value we're looking for at the end (data) is in fact the value we spuriously get at the very beginning already from openWindow().

I guess some of the in-between stages no longer exist somehow.

It's looking more and more like a bug in jQuery's Deferred stuff. I've distilled it to this code that can be pasted into the browser's console:

var openingDeferred;

var getWindowDeferred = $.Deferred();

var openingPromise = getWindowDeferred.promise().then( function ( win ) {
    console.log( 'getWindowDeferred resolved with ' + win );
    openingDeferred = $.Deferred();
    return openingDeferred.promise();
} ).promise();
openingPromise.XXX = 'openingPromise';

// Here's the chain of thens in windows.js
openingPromise.then( function ( opened ) {
    console.log( openingPromise.XXX + ' resolved with ' + opened.XXX );
    if ( !opened.then ) {
        return '"opened" is not a promise';
    }
    return opened.then( function ( closed ) {
        console.log( opened.XXX + ' resolved with ' + closed.XXX );
        return $.Deferred().resolve( 'final-value' );
    } );
} ).then( function ( x ) {
    console.log( 'Final value is', x );
} );

// There's some more promises and setTimeouts before the other Deferreds start
// getting resolved.
setTimeout( function () {
    console.log( 'Resolving getWindowDeferred' );
    getWindowDeferred.resolve( 'A window' );

    setTimeout( function () {
        console.log( 'Resolving openingDeferred' );
        var closedDeferred = $.Deferred();
        var closedPromise = closedDeferred.promise();
        closedPromise.XXX = 'closedPromise';
        openingDeferred.resolve( closedPromise );

        setTimeout( function () {
            console.log( 'Resolving closedDeferred' );
            closedDeferred.resolve( { XXX: 'Data' } );
        }, 1000 );
    }, 1000 );
}, 1000 );

In 1.11, it works out as expected

Resolving getWindowDeferred 
getWindowDeferred resolved with A window 
Resolving openingDeferred 
openingPromise resolved with closedPromise 
Resolving closedDeferred 
closedPromise resolved with Data 
Final value is final-value

In 3.2.1, however, it exhibits the same buggy behavior seen in OO.ui.confirm:

Resolving getWindowDeferred 
getWindowDeferred resolved with A window 
Resolving openingDeferred 
Resolving closedDeferred 
openingPromise resolved with Data 
Final value is "opened" is not a promise

Note how in the 3.2.1 case we don't get the "openingPromise resolved" message until after "Resolving closedDeferred", in addition to it having the wrong value.

We can do it the other way too:

var openingDeferred;

var getWindowDeferred = $.Deferred();

var openingPromise = getWindowDeferred.promise().then( function ( win ) {
    console.log( 'getWindowDeferred resolved with ' + win );
    openingDeferred = $.Deferred();
    return openingDeferred.promise();
} ).promise();
openingPromise.XXX = 'openingPromise';

// Here's the chain of thens in windows.js
openingPromise.then( function ( opened ) {
    console.log( 'openingPromise resolved with ' + opened.XXX );
    return opened;
} ).then( function ( closed ) {
    console.log( '"opened" resolved with ' + closed.XXX );
    return $.Deferred().resolve( 'final-value' );
} ).then( function ( x ) {
    console.log( 'Final value is', x );
} );

// There's some more promises and setTimeouts before the other Deferreds start
// getting resolved.
setTimeout( function () {
    console.log( 'Resolving getWindowDeferred' );
    getWindowDeferred.resolve( 'A window' );

    setTimeout( function () {
        console.log( 'Resolving openingDeferred' );
        var closedDeferred = $.Deferred();
        var closedPromise = closedDeferred.promise();
        closedPromise.XXX = 'closedPromise';
        openingDeferred.resolve( closedPromise );

        setTimeout( function () {
            console.log( 'Resolving closedDeferred' );
            closedDeferred.resolve( { XXX: 'Data' } );
        }, 1000 );
    }, 1000 );
}, 1000 );

In 1.11:

Resolving getWindowDeferred 
getWindowDeferred resolved with A window 
Resolving openingDeferred 
openingPromise resolved with closedPromise 
Resolving closedDeferred 
"opened" resolved with Data 
Final value is final-value

In 3.2.1:

Resolving getWindowDeferred 
getWindowDeferred resolved with A window 
Resolving openingDeferred 
Resolving closedDeferred 
openingPromise resolved with Data 
"opened" resolved with Data 
Final value is final-value

Again in the 3.2.1 case we don't get the "openingPromise resolved" message until after "Resolving closedDeferred".

I'm skeptical of the Identity function used in jQuery deferred.js, my guess is that under these conditions it's incorrectly being interpreted as a real 'then' instead of whatever purpose it's supposed to really be serving in there. If I replace the Identity in that linked line with a different function returning null (and also update line 175 to test for the different function in addition to Identity), 3.2.1 starts working correctly. But I don't understand that code well enough to understand how it's going wrong with Identity there.

@Anomie Nice. I've generalised it a bit into this CodePen: https://codepen.io/Krinkle/pen/JNbNzg?editors=0010

Reported upstream at https://github.com/jquery/jquery/issues/3646.

Thanks for reporting it upstream.

I'm not a 100% sure whether this was meant to be supported in the past.

I note this behavior only happens when you resolve a promise with another promise (or probably with any thenable) as its first argument. Resolve it with the new promise as a non-first argument, or with an object or array that contains the new promise, and 3.x still works like 1.x and 2.x.

And it only happens after the first 'then' in a promise.then().then() chain. See for example https://codepen.io/anon/pen/mmOMvg?editors=0010 which does the opened-closing-closed without the getWindow in there, and https://codepen.io/anon/pen/PmbJZw?editors=0010 that does the alternative method of chaining that I mentioned in T163510#3200488.

On the other hand, https://promisesaplus.com/#the-promise-resolution-procedure seems like this new behavior is the intended behavior if jQuery 3 is following that spec now, in which case jQuery is buggy in not doing it for the simple promise.resolve( promise2 ) case.

d1 = $.Deferred();
d2 = $.Deferred();
d1.promise().then( () => console.log( "d1 resolved" ) );
d1.resolve( d2.promise() );
// Should not have logged d1 being resolved, but 3.2.1 does

Indeed. So we found a bug, but the bug is that both A and B should break, instead of both pass.

Let's work around, then?

So, it shouldn't be possible to resolve a promise with another promise? Is there a way to work around this, other than wrapping the value in some object, which would be a breaking change for OOjs UI?

In T163510#3214427, @matmarex wrote:

So, it shouldn't be possible to resolve a promise with another promise?

According to whoever wrote Promises/A+, yes. They decided that when you do that, you actually meant to return promise1.then( function () { return promise2; } ) instead of returning promise1 directly.

Is there a way to work around this, other than wrapping the value in some object, which would be a breaking change for OOjs UI?

Pass it as a not-first value to resolve(), which jQuery still allows despite Promises/A+ only allowing a single value. But that would be a breaking change too.

Use something other than jQuery's Deferred/Promise that doesn't have this behavior. Potentially just a subclass/wrapper where resolve() wraps the promise in an object before calling jQuery.Deferred's resolve() or inserts a dummy first parameter, and then() wraps the passed callback with a function that first unwraps the value. That seems potentially fragile, but would avoid a breaking change.

(If we end up making a breaking change because of this, we might want to come up with an entirely different API for waiting for a dialog to open/close. Multiple people told me that the chained promises we have are extremely weird.)

Change 350789 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[oojs/ui@master] [WIP] Fix OO.ui.confirm() for jQuery 3

https://gerrit.wikimedia.org/r/350789

In T163510#3200488, @Anomie wrote:

I have no idea why, but changing the promise chaining to

.then( function ( opened ) {
    return opened;
} ).then( function ( closing ) {
    return closing;
} ).then( function ( data ) {
    return $.Deferred().resolve( !!( data && data.action === 'accept' ) );
} );

seems to work, although not completely correctly.

In what way not correctly? Looks fine to me. Documented it more elaborately in https://gerrit.wikimedia.org/r/350789.

In T163510#3219921, @Krinkle wrote:

In T163510#3200488, @Anomie wrote:

I have no idea why, but changing the promise chaining to

.then( function ( opened ) {
    return opened;
} ).then( function ( closing ) {
    return closing;
} ).then( function ( data ) {
    return $.Deferred().resolve( !!( data && data.action === 'accept' ) );
} );

seems to work, although not completely correctly.

In what way not correctly? Looks fine to me. Documented it more elaborately in https://gerrit.wikimedia.org/r/350789.

It still delays running the whole chain until the final promise is resolved, and the opening "then" is still passed the final data rather than the promise it's supposed to get. See https://codepen.io/anon/pen/PmbJZw?editors=0010.

In retrospect, "seems to work, although not completely correctly" wasn't a correct statement. It works for the very simple case where you're just returning the promise at each intermediate step without doing anything else, but if you actually depend on 'opened' or 'closing' being promises or being called at the right times it breaks.

The names we use for the promises are a bit confusing, but here is my current understanding:

• OO.ui.WindowManager#openWindow() returns a promise nicknamed "opening".
• This "opening" promise is resolved once the window has finished opening (it waits for any previous window to close, and then the new window to be "setup" and "ready").
• The "opening" promise resolution is also given a placeholder Deferred nicknamed "opened".
• The "opened" promise is resolved when WindowManager#closeWindow() is called.
• The "opened" promise resolution is also given a placeholder Deferred nicknamed "closing".
• The "closing" promise is resolved once resolved once the window has finished closing, and waits also for any "hold" and "teardown".
• The "closing" promise resolution is also given any closing data, as it was passed to closeWindow().

Another less-confusing way of looking at this (after reading the documentation):

• openWindow() returns a promise for the "opening" event, emitted when the window begins to open. It is given a promise for when the opening has finished.
• closeWindow() returns a promise for the "closing" event, emitted when the window begins to close. It is given a promise for when the closing has finished.
• In addition, the "opened" promise is given a pending promise that will be resolved when closeWindow() is called.

Problems

When trying to come up with a solution, various pre-existing problems become clear:

• The Window object is lacking any events for opening and closing. They are only fired on the WindowManager. However, these don't seem useful for callers of getWindow or openWindow since don't want events about just any window, only theirs.
• WindowManager#openWindow() does not provide a reference to the Window object because it may be created asynchronously via getWindow() in some cases.
• The only reference to the Window object is through the event listeners or by calling getWindow() first instead of openWindow().

In addition, even discarding the fact that WindowManager events are not useful for callers of openWindow (since they may be about other windows), using it would be awkward as you'd have to go like this:

• Call openWindow() - use the promise to catch errors but otherwise ignore.
• Add a once() listener to WindowManager for closing event.
• When fired, call .then() on its closing parameter which is a Promise for when the closing has finished.
• Inside that then handler, you now have the closing data.

I can only imagine the code required to make this work and various ways in which will be unpleasant to use.

Solution?

Some thoughts for a proposal to fix this:

• openWindow() and closeWindow() methods keeps emitting the same event with the same parameters.
• closeWindow() still returns the same promise.
• (BREAKING CHANGE) The openWindow()'s "opening" promise resolution will change from two arguments to one argument. Before: (Promise opened, openingData). After: (openingData).
• (The internal "opened" promise is removed.)

One idea:

• Add open and close events to the Window object.
• Also add "opened" and "closed" events for when these actions complete, so that callers don't have to use an event listener to get a promise.
• Also add events for the other intermediate "notifications" the current promises provide ("setup", "ready", "hold", "teardown").
• Also add an "error" event for when either of these actions fail.

The thing that openWindow() is unintentionally doing in jQuery 3 - and the thing that high-level methods like OO.ui.confirm() need - would then basically look like typical "Promise for a once event" code, as follows:

OO.ui.confirm = function ( text ) {
  return windowManager.getWindow( 'message',  { message: text } ).then( function ( win ) {
    windowManager.openWindow( win );
    return $.Deferred( function ( d ) {
      win.once( 'closed', d.resolve );
      win.once( 'error', d.reject );
    } );
  } );
};

I'm ok with changing these APIs and even breaking changes if necessary. Bear in mind the major consumer of dialogs is still VE, and we mostly create windows by extending the dialog class, which is how they were designed. The fact that we do confirm/alert/prompt without extending is a little hacky.

Okay, I've started the refactoring at https://gerrit.wikimedia.org/r/350789. It should be straight forward to maintain the current code for a few release cycles before jQuery 3 goes live so that we can find more usage of the to-be-deprecated methods in our code bases, which is hard to do with grep, so we'll likely need run-time warnings to detect it.

CCing @TrevorParscal as the author, and @dchan for his opinions on events.

Change 352538 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[oojs/ui@master] windows: Update OO.ui.alert/confirm/prompt to use Window events

https://gerrit.wikimedia.org/r/352538

Also noticed that the languageInputWidget is broken. It does manager.openWindow().then( function ( opened ) {}... but instead of getting a promise - it moves on to the next step and gives you the closing data.

Similarly, the transclusion dialog fails after you hit "insert", for what seems to be related reasons.

Change 350789 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Add WindowInstance - a Promise-based lifecycle object

https://gerrit.wikimedia.org/r/350789

Change 352538 merged by Bartosz Dziewoński:
[oojs/ui@master] windows: Add tests for OO.ui.alert/confirm/prompt

https://gerrit.wikimedia.org/r/352538

Return value in docblock of [[ https://github.com/wikimedia/oojs-ui/blob/master/src/Window.js | Window::close ]] should be updated accordingly, copying the return value from docblock of [[ https://github.com/wikimedia/oojs-ui/blob/master/src/WindowManager.js | WindowManager::closeWindow ]].

Same for docblock of [[ https://github.com/wikimedia/oojs-ui/blob/master/src/Window.js | Window::open ]], which should be updated according to [[ https://github.com/wikimedia/oojs-ui/blob/master/src/WindowManager.js | WindowManager::openWindow ]].

Fixed in https://gerrit.wikimedia.org/r/456059. Thanks.