Skip to content
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

Merged
merged 11 commits into from
Mar 16, 2024

Conversation

kriskowal
Copy link
Member

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 the cancelled endowment because I could not in good conscience claim the global id, and this made the two names consistent.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

  • Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
  • Updates NEWS.md for user-facing changes.

@kriskowal kriskowal requested a review from rekmarks March 15, 2024 01:42
@kriskowal kriskowal force-pushed the kriskowal-endo-reconstruct-weblets-1921 branch 2 times, most recently from b89aee5 to a886ef1 Compare March 15, 2024 03:47
Copy link
Contributor

@rekmarks rekmarks left a 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 () => {
Copy link
Contributor

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?

Copy link
Member Author

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/daemon/src/web-server-node-powers.js Outdated Show resolved Hide resolved
packages/daemon/src/web-server-node.js Show resolved Hide resolved
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong parameter order:

Suggested change
> endo install familiar-chat cat.js 8920 --powers SELF
> endo install familiar-chat 8920 cat.js --powers SELF

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kriskowal kriskowal force-pushed the kriskowal-endo-reconstruct-weblets-1921 branch from bd71c50 to 2d946cf Compare March 16, 2024 03:27
@kriskowal kriskowal force-pushed the kriskowal-endo-reconstruct-weblets-1921 branch from 2d946cf to 3fa3d9b Compare March 16, 2024 03:27
@kriskowal kriskowal enabled auto-merge March 16, 2024 03:29
@kriskowal kriskowal merged commit 5fadd4a into master Mar 16, 2024
18 checks passed
@kriskowal kriskowal deleted the kriskowal-endo-reconstruct-weblets-1921 branch March 16, 2024 03:35
rekmarks added a commit that referenced this pull request Mar 18, 2024
Completes our error handling logic for the daemon's web server following #2148.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS Sonoma leaves *.localhost handling at the mercy of ISPs
2 participants