Conversation
|
Hey there! I just built a new temporary npm package based on 3c2e08f. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251209172634.tgz |
|
Hey there! I just built a new temporary npm package based on a9e1006. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251209183439.tgz |
|
Hey there! I just built a new temporary npm package based on 94815bf. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251215174815.tgz |
|
Hey there! I just built a new temporary npm package based on 7692e40. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251216203205.tgz |
|
Hey there! I just built a new temporary npm package based on a826f2b. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251216203632.tgz |
There was a problem hiding this comment.
Pull request overview
This PR adds support for running promises within Roku Task nodes by introducing an event loop mechanism that allows tasks to observe and handle promise state changes through message ports.
Key Changes
- Added
runEventLoopandinitTaskFunctionalityfunctions to enable promise support in tasks - Modified promise observation logic to conditionally use message ports in task contexts
- Added test cases demonstrating promise usage in tasks with various scenarios
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/source/promises.bs | Core implementation adding task event loop and conditional observation logic |
| src/source/promises.spec.bs | Reformatted indentation (tabs to spaces) and added three task-based test cases |
| src/components/TestTask.spec.xml | New task component definition for testing promise functionality |
| src/components/TestTask.spec.bs | Task implementation with test methods for promise resolution, network calls, and rejection |
| package.json & package-lock.json | Updated brighterscript dependency from 0.69.10 to 0.69.11 |
| demos/simple-brightscript/* | Added demo task component and updated MainScene to demonstrate task usage |
| bsconfig.tests.json & .vscode/settings.json | Minor configuration updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey there! I just built a new temporary npm package based on ec7b3e9. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251218134602.tgz |
|
Hey there! I just built a new temporary npm package based on 6f99082. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251219115730.tgz |
|
Hey there! I just built a new temporary npm package based on 0b0273f. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251219135928.tgz |
|
Hey there! I just built a new temporary npm package based on e359c47. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20251219144557.tgz |
|
Hey there! I just built a new temporary npm package based on a575e9c. You can download it here or install it by running the following command: npm install https://github.com/rokucommunity/promises/releases/download/v0.0.0-packages/rokucommunity-promises-0.6.7-65-support-observing-promises-in-tasks.20260408124233.tgz |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function peekMessage(port as dynamic) as dynamic | ||
| while true | ||
| message = port.peekMessage() | ||
|
|
||
| 'if this is a promise event, process it and peek again | ||
| if promises.isPromiseEvent(message) then | ||
| 'remove the message from the queue and process it | ||
| message = port.getMessage() | ||
| promises.internal.notifyListeners(message) |
There was a problem hiding this comment.
peekMessage() assumes port is a valid roMessagePort. If promises.getMessagePort() hasn't been set (or an invalid port is passed), calling port.peekMessage()/port.getMessage() will throw. Consider guarding port = invalid (and/or type(port) <> "roMessagePort") and returning invalid in that case, and ensure callers of wait2() don't invoke promises.peekMessage() with an invalid promise port.
| else | ||
| thisTickTimeout = timeoutMilliseconds |
There was a problem hiding this comment.
wait2()'s tick-timeout calculation overwrites thisTickTimeout with timeoutMilliseconds in the else branch, which defeats the remaining-time calculation and can cause waits longer than the overall timeout. It looks like the intent was to keep the smaller remaining thisTickTimeout when it's <= 200ms.
| else | |
| thisTickTimeout = timeoutMilliseconds |
| 'if the port we're waiting on is the promises port, then no need for the double-latch | ||
| if portIsPromisesPort then | ||
| event = wait(timeoutMilliseconds, port) | ||
| if promises.isPromiseEvent(event) then | ||
| promises.internal.notifyListeners(event) | ||
| continue while | ||
| end if | ||
| return event |
There was a problem hiding this comment.
In the portIsPromisesPort path, wait2() uses wait(timeoutMilliseconds, port) each loop iteration. This can exceed the overall timeout because it doesn't use the computed remaining thisTickTimeout (and effectively resets the wait each time a promise event is handled). Using wait(thisTickTimeout, port) would preserve native wait() semantics.
| if promises.internal.hasStorage(originalPromise) then | ||
| ' There were listeners added as a result of some of the callback notifications | ||
| ' Re-trigger the notification process for the new listeners | ||
| promises.internal.delay(sub (event as object) | ||
| if m.__promises__promisesPort <> invalid then | ||
| promises.internal.notifyListeners(event) | ||
| end sub, event) | ||
| else | ||
| promises.internal.delay(sub (event as object) | ||
| promises.internal.notifyListeners(event) | ||
| end sub, event) | ||
| end if |
There was a problem hiding this comment.
When m.__promises__promisesPort is set, notifyListeners immediately recurses if new listeners were added during callback processing. This removes the previous delay(...) protection against deep synchronous recursion/stack overflows (especially in large chains). Consider keeping the delayed re-notification (or enqueueing work via the message port) even when using the message-port observation mode.
| requestId = message.getData() | ||
| promises.resolve({ ipInfo: "hello" }, m.requestStorage[requestId].promise) |
There was a problem hiding this comment.
requestId is actually the new value of the request field (currently a URL string), since m.requestStorage is keyed by url. Renaming this local to something like requestUrl (and/or keying storage by an actual generated id) would make the intent clearer and reduce confusion.
| requestId = message.getData() | |
| promises.resolve({ ipInfo: "hello" }, m.requestStorage[requestId].promise) | |
| requestUrl = message.getData() | |
| promises.resolve({ ipInfo: "hello" }, m.requestStorage[requestUrl].promise) |
| @@ -0,0 +1,8 @@ | |||
| <?rokuml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
XML declaration appears to be misspelled as <?rokuml ...?>, which will make this file invalid XML and prevent the component from loading. This should be <?xml version="1.0" encoding="utf-8"?> (or removed entirely).
| <?rokuml version="1.0" encoding="utf-8"?> | |
| <?xml version="1.0" encoding="utf-8"?> |
| promises.initTaskFunctionality(m.port) | ||
| promise = doNetworkRequest("http://ip-api.com/json/", "GET", {}) | ||
| promises.chain(promise).then(function(response) | ||
| m.top.response = response | ||
| end function).catch(function(error) | ||
| print "Error in TaskWithInternalPromises:", error | ||
| end function) | ||
|
|
||
| promises.runEventLoop(m.port, sub(message) | ||
| print "Task with internal promises received message:", message | ||
| end sub) |
There was a problem hiding this comment.
promises.initTaskFunctionality and promises.runEventLoop are called here, but no such functions exist in src/source/promises.bs (searching the repo shows no definitions). This demo won't run as-is; either add these APIs to the library or update the demo to use the actual public functions (setMessagePort, wait2, etc.).
| promises.initTaskFunctionality(m.port) | |
| promise = doNetworkRequest("http://ip-api.com/json/", "GET", {}) | |
| promises.chain(promise).then(function(response) | |
| m.top.response = response | |
| end function).catch(function(error) | |
| print "Error in TaskWithInternalPromises:", error | |
| end function) | |
| promises.runEventLoop(m.port, sub(message) | |
| print "Task with internal promises received message:", message | |
| end sub) | |
| promises.setMessagePort(m.port) | |
| m.done = false | |
| promise = doNetworkRequest("http://ip-api.com/json/", "GET", {}) | |
| promises.chain(promise).then(function(response) | |
| m.top.response = response | |
| m.done = true | |
| end function).catch(function(error) | |
| print "Error in TaskWithInternalPromises:", error | |
| m.done = true | |
| end function) | |
| while not m.done | |
| message = wait(0, m.port) | |
| if message <> invalid | |
| print "Task with internal promises received message:", message | |
| end if | |
| end while |
| return "Fiji is " + fijiHour.toStr() + " hours " + aheadOrBehind + " of your timezone!" | ||
| timezone = data.timezone | ||
| doy = data.day_of_year | ||
| return "Confirming your timezone is is " + timezone + " and the day of year is " + doy.toStr() + "!" |
There was a problem hiding this comment.
Typo in user-facing string: "timezone is is" has a duplicated "is".
| return "Confirming your timezone is is " + timezone + " and the day of year is " + doy.toStr() + "!" | |
| return "Confirming your timezone is " + timezone + " and the day of year is " + doy.toStr() + "!" |
| promises.chain(promise).then(function(result) | ||
| m.testSuite.assertEqual(result, "task completed") | ||
| m.testSuite.done() | ||
| end function).catch(function(error) | ||
| print "error", error, FormatJson(error.backtrace) | ||
| end function) |
There was a problem hiding this comment.
This async test's .catch(...) handler only logs; it never calls m.testSuite.fail(...)/m.testSuite.done(). If the task promise rejects, the test run will hang instead of reporting a failure. Please ensure the catch path fails the test and completes it.
| promises.chain(promise).then(function(result) | ||
| m.testSuite.assertEqual(result.ipInfo, "hello") | ||
| m.testSuite.done() | ||
| end function).catch(function(error) | ||
| print "error", error, FormatJson(error.backtrace) | ||
| end function) |
There was a problem hiding this comment.
This async test's .catch(...) handler only logs; it never calls m.testSuite.fail(...)/m.testSuite.done(). If the task promise rejects, the test run will hang instead of reporting a failure. Please ensure the catch path fails the test and completes it.
No description provided.