feat: return Promises for Ti.Window open()/close() methods #12407
Conversation
|
After a lot of messing around my best guess here is that internal code is calling window close, and is not "consuming" the promise generated (because it's not actually surfaced to the JS engine/user) and JSC is freaking out because we have unhandled promise rejections/fulfillments. So... I'm not sure what the "fix" here would be. Perhaps to explicitly delineate the JS-facing method from the method to be used internally? Flag in some way so the |
| first.then(() => { | ||
| const second = tabGroup.open(); | ||
| second.should.be.a.Promise(); | ||
| second.then(() => finish(new Error('Expected second #open() to be rejected'))).catch(_e => finish()); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.tabgroup.test.js line 468 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.tabgroup.test.js line 468 – Avoid nesting promises. (promise/no-nesting)
| backgroundColor: '#0000ff' | ||
| }); | ||
|
|
||
| win.open().then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.window.test.js line 718 – Each then() should return a value or throw (promise/always-return)
Tests:
|
|
@sgtcoolguy Why would you add a tab group to a navigation window? They are both top-level containers and not meant to be added to each other, which would explain the crash. I hope that helps :) |
|
@hansemannn developers do crazy things. It's in our test suite as a regression check because we have customers who have that code! Anyways, I narrowed this down eventually. The crash is specifically coming from when the old style The second thing which was my initial hunch is that if I create a |
|
Customers … 😄 |
| tabGroup.addTab(tabB); | ||
|
|
||
| const openPromise = tabGroup.open(); | ||
| openPromise.then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.tabgroup.test.js line 399 – Each then() should return a value or throw (promise/always-return)
|
|
||
| const first = nav.open(); | ||
| first.should.be.a.Promise(); | ||
| first.then(() => nav.open(), e => finish(e)).then(() => finish(new Error('Expected second #open() to be rejected')), _e => finish()); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 100 – Expected catch() or return (promise/catch-or-return)
| }); | ||
|
|
||
| const openPromise = win.open(); | ||
| openPromise.then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.window.test.js line 696 – Each then() should return a value or throw (promise/always-return)
| openPromise.then(() => { | ||
| const result = win.close(); | ||
| result.should.be.a.Promise(); | ||
| result.then(() => finish()).catch(e => finish(e)); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.window.test.js line 699 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.window.test.js line 699 – Avoid nesting promises. (promise/no-nesting)
|
|
||
| const first = win.open(); | ||
| first.should.be.a.Promise(); | ||
| first.then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.window.test.js line 752 – Each then() should return a value or throw (promise/always-return)
sgtcoolguy
force-pushed
the
window-promises
branch
from
8a102d3
to
fb97a16
Compare
sgtcoolguy
changed the title
sgtcoolguy
force-pushed
the
window-promises
branch
from
fb97a16
to
528a79c
Compare
| nav = Ti.UI.createNavigationWindow({ window: Ti.UI.createWindow() }); | ||
|
|
||
| nav.open().then(() => { | ||
| nav.close().then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 68 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 68 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 68 – Each then() should return a value or throw (promise/always-return)
|
|
||
| nav.open().then(() => { | ||
| nav.close().then(() => { | ||
| nav.close().then(() => finish(new Error('Expected second #close() call on Window to be rejected'))).catch(e => finish()); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 69 – 'e' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 69 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 69 – Avoid nesting promises. (promise/no-nesting)
| openPromise.then(() => { | ||
| const result = tabGroup.close(); | ||
| result.should.be.a.Promise(); | ||
| result.then(() => finish()).catch(e => finish(e)); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.tabgroup.test.js line 402 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.tabgroup.test.js line 402 – Avoid nesting promises. (promise/no-nesting)
| openPromise.then(() => { | ||
| const result = nav.close(); | ||
| result.should.be.a.Promise(); | ||
| result.then(() => finish()).catch(e => finish(e)); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 52 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 52 – Avoid nesting promises. (promise/no-nesting)
sgtcoolguy
force-pushed
the
window-promises
branch
from
166500a
to
708b6a0
Compare
|
|
||
| const result = nav.open(); | ||
| result.should.be.a.Promise(); | ||
| result.then(() => finish(), e => finish(e)); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 92 – Expected catch() or return (promise/catch-or-return)
| first.then(() => { | ||
| const second = win.open(); | ||
| second.should.be.a.Promise(); | ||
| second.then(() => finish(new Error('Expected second #open() to be rejected'))).catch(_e => finish()); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.window.test.js line 755 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.window.test.js line 755 – Avoid nesting promises. (promise/no-nesting)
| nav = Ti.UI.createNavigationWindow({ window: Ti.UI.createWindow() }); | ||
|
|
||
| const openPromise = nav.open(); | ||
| openPromise.then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 49 – Each then() should return a value or throw (promise/always-return)
sgtcoolguy
force-pushed
the
window-promises
branch
from
708b6a0
to
3748e8a
Compare
| it('called twice on Window rejects second Promise', finish => { | ||
| nav = Ti.UI.createNavigationWindow({ window: Ti.UI.createWindow() }); | ||
|
|
||
| nav.open().then(() => { |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.navigationwindow.test.js line 67 – Each then() should return a value or throw (promise/always-return)
|
|
||
| win.open().then(() => { | ||
| win.close().then(() => { | ||
| win.close().then(() => finish(new Error('Expected second #close() call on Window to be rejected'))).catch(e => finish()); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.window.test.js line 720 – 'e' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)⚠️ tests/Resources/ti.ui.window.test.js line 720 – Avoid nesting promises. (promise/no-nesting)⚠️ tests/Resources/ti.ui.window.test.js line 720 – Avoid nesting promises. (promise/no-nesting)
|
I don't think this is a good idea overall. Even though I like |
|
@drauggres Why specifically do you disagree here? In doing this implementation, it struck me by how often we actually have hidden cases of failures or "ignoring" the open/close calls we do where we simply spit something to the logs (or not even that!) and there's no means of determining concretely whether or not the open or close call worked. In the past developers are expected to hang event listeners to get notified if they do get opened/closed - but there was really no mechanism for handling failures specifically. I do think this will force us to deal with the overall problem of generic unhandled promise rejections - which we'll run into regardless of this PR, but obviously with lots of pre-existing code out there this would quickly trigger for existing apps. But presuming we can hook a global event and default listener to log unhandled promises, this should actually help developers pinpoint where they may have open/close calls occurring in bad states or with timing issues. |
* properly resolve Ti.UI.NavigationWindow.close() * use constant reference for close event name * move rejection of closing unopened window up hierarchy so it applies to TabGroup too
sgtcoolguy
force-pushed
the
window-promises
branch
from
3748e8a
to
1566bbd
Compare
| done(); | ||
| }); | ||
| win.close(); | ||
| win.close().then(() => done()).catch(_e => done()); |
There was a problem hiding this comment.
⚠️ tests/Resources/ti.ui.view.test.js line 31 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)⚠️ tests/Resources/ti.ui.view.test.js line 31 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
|
This has caused TIMOB-28394. Reverting this commit fixes the issue. Please go through a proper code-review :( |
JIRA: #12407
Description:
Relates to #12349, #12407, #12417
This modifies some of our async methods to also return a Promise:
Ti.UI.Window.open()Ti.UI.Window.close()Ti.UI.NavigationWindow.open()Ti.UI.NavigationWindow.close()Ti.UI.TabGroup.open()Ti.UI.TabGroup.close()Unit tests are included!