-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(daemon,cli): Serve one weblet per fixed HTTP port. #2148
Conversation
b89aee5
to
a886ef1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One requested change (param order in docs), and some optional miscellanea.
Oh, and yarn docs
is unhappy.
|
||
if (respond) { | ||
server.on('request', (req, res) => { | ||
(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of the IIFE? The callback itself can't be async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An async callback produces a dangling promise, which means the error can only surface as an unhandled rejection. Exceptions should be deliberately observed.
packages/cli/demo/README.md
Outdated
@@ -257,7 +257,7 @@ _Familiar Chat_ is an example application that provides a web app for | |||
interacting with your pet daemon. | |||
|
|||
``` | |||
> endo install familiar-chat cat.js --powers SELF | |||
> endo install familiar-chat cat.js 8920 --powers SELF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong parameter order:
> endo install familiar-chat cat.js 8920 --powers SELF | |
> endo install familiar-chat 8920 cat.js --powers SELF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In light of this, perhaps we should add some parameter validation in the CLI for the name, port, and program path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and changed --name
and --port
to required flags instead of a positional arguments.
a886ef1
to
bd71c50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
bd71c50
to
2d946cf
Compare
2d946cf
to
3fa3d9b
Compare
Completes our error handling logic for the daemon's web server following #2148.
closes: #1921
Description
This stack of individually reviewable commits fully decouples the platform-specific weblet implementation from the platform-agnostic daemon, turning it into a Node.js-specific unconfined worklet.
Removing the allegëd
type
from formula identifiers entirely increased the urgency of fixing #1921. The new weblet architecture does not require a special web bundle type or any of the other weird coupling issues they have with wildcard subdomains. That refactor is bundled with this fix.Also motivated by weblets,
eval
formulas now receive both$cancelled
and$id
endowments. The$id
endowment is necessary for weblets to recognize their own access token, a path constructed from a prefix of their own formula identifier. I’ve renamed thecancelled
endowment because I could not in good conscience claim the globalid
, and this made the two names consistent.Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Compatibility Considerations
Upgrade Considerations
*BREAKING*:
in the commit message with migration instructions for any breaking change.NEWS.md
for user-facing changes.