Align initial Platform implementation with spec - #132
Conversation
dolanske
changed the title
zealsprince
left a comment
zealsprince
left a comment
There was a problem hiding this comment.
The tray reframing is the best part of this PR. Awesome! Once you say it out loud, badge count plus alert dot plus title really is the same contract on every target, and the spec's own capability table at 06-platform.md line 56 already describes exactly this for web (document.title badge count, favicon overlay). It's the interface snippet a few lines up that says tray: TrayPort | null.
So your change is more consistent with the spec than the spec is with itself, and I'll get orbit-spec updated to match unless you'd rather fold it in. The @targets doc comments are also exactly what #36 wanted.
Three things I'd want fixed before this merges:
- I want to confirm the badge actually draws red :)
- The mock's
notifyconstructs a realNotification, which throwsReferenceErrorin a DOM-less env. That's the one thing #36 explicitly asked the mock not to do. OrbitApp.vuehas "Did you know that" as the loading copy.
Two open questions:
-
historyCachewas listed as out of scope on #36, with #10 owning it. This PR lands the fullHistoryCachePortinterface, the mock stubs, and acreateIndexedDbCachePortTODO. I'm not against claiming the shape early, but it means #10 inherits a contract it didn't write, and the mock returning empty arrays fromseedmeans anything wired to it shows no history with no error. Would you rather strip it back out and let #10 define it or keep it? AlsocreateIndexedDbCachePortbuilds a whole mock platform just to pull one port off it, which acreateMockHistoryCacheexport frommock.tswould tidy up. -
createDesktopPlatformreturning the mock is the one I keep coming back to. #36 was about making null mean something, and this makes desktop claim every capability while doing nothing. Silent noops on a target nobody's testing yet feels like the failure mode we were trying to design out.
| fileTransfer: null, | ||
| dns: null, | ||
| } | ||
| return createMockPlatform("desktop") |
There was a problem hiding this comment.
Do we want desktop returning the mock? #36's whole framing was making null mean "this target can't do this", and now desktop reports every port as present when it actually does nothing. A tray badge that never shows up with no error is a lot harder to chase than a null check I think. Could createDesktopPlatform keep returning null per port (or throw) until the Tauri side lands, so the gap stays visible?
There was a problem hiding this comment.
This change would have to make all platforms fields nullable though. Which would affect the web platform implementation, since we'd have to check for fields, which in the future we'll 100% have available on all platforms.
It feels like the correct response is to completely nuke the desktop folder, because platform is wired right into orbit initialization function (see createOrbitApp)
Obviously "let's ignore the leaking toilet until we have to use it" approach does not feel right, but I don't really know what's the simplest way to fix that. Any ideas?
There was a problem hiding this comment.
I don't think we need to touch nullability for this. Desktop runs in a webview, so we could spread createWebPlatform() and just override target: "desktop". Title, mediaDevices and the anchor download all should genuinely work in a Tauri window, the notification port already degrades to false when Notification isn't around, and deepLinks / dns stay null since those are the two that need Rust. That keeps the "null means can't do this" reading from #36 without the web adapter having to guard fields it'll always have. The spec's own line for new targets is "reuse an existing adapter and override only what differs", which is pretty much this. The one port that would misbehave is the favicon overlay, since there's no tab to draw on, so I'd leave a TODO on it as the first thing the Tauri ticket overrides. I'd rather not nuke apps/desktop either. It's the shell the Tauri work lands in, and desktop implementations are already their own ticket per #36
| it("Should contain nullable fields on desktop", () => { | ||
| const mocked = createMockPlatform("desktop") | ||
| expect(mocked.deepLinks).toBeDefined() | ||
| expect(mocked.deepLinks.onOpen).toBeDefined() |
There was a problem hiding this comment.
Worth knowing the reason line 40 doesn't error on a DeepLinkPort | null is that tsconfig.app.json only includes src/**/*.ts, so vue-tsc -b never sees the test files at all.
There was a problem hiding this comment.
Is there anything I can do to improve the situation here? I might be having trouble understanding this. I get that TS might not be evaluating test files right now, but after adding .test.ts into the settings, nothing changed in my IDE
There was a problem hiding this comment.
The problem is that it's including src/**/*.ts but never packages/**/*.ts. Also, I don't think your rule works because it's just checking for .test.ts files. If anything, it would have to be **/*.test.ts to match all test files.
2 participants
This PR implements the web platform controller based on #36
It intentionally leaves out history and desktop implementation - the idea is to have it aligned with the spec as much as possible to get the ball moving, without getting too much into the weeds. At least that's how I understood it.
This PR also adds support for
trayon the web. After some thought, I realized thattraydoes pretty much the same thing in all environments.Side effect of this was that I had to upload a favicon and I also removed some random HTML jank. Hopefully this does not stray much from the task's core.
Additionally, it adds a mock platform implementation and tests against it. Improves a couple types and adds simple comments explaining what platforms are these methods available on.