Allow PubSub to Use External Instance#828
Open
KetanReddy wants to merge 9 commits intoplayer-1-dot-zerofrom
Open
Allow PubSub to Use External Instance#828KetanReddy wants to merge 9 commits intoplayer-1-dot-zerofrom
KetanReddy wants to merge 9 commits intoplayer-1-dot-zerofrom
Conversation
Co-authored-by: Kelly Harrop <kn.harrop@gmail.com>
…ts (#827) * Add CLAUDE.md with guidelines for reporting and reviewing pull requests * Fix: Include CONTRIBUTORS in Claude Code Review job conditions
* Cancels the already-running scroll animation on page transition
KetanReddy
changed the title
KetanReddy
changed the title
Bundle ReportChanges will increase total bundle size by 9.24kB (0.16%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: plugins/reference-assets/coreAssets Changed:
view changes for bundle: core/playerAssets Changed:
view changes for bundle: plugins/auto-scroll/reactAssets Changed:
view changes for bundle: plugins/metrics/coreAssets Changed:
view changes for bundle: core/partial-match-registryAssets Changed:
view changes for bundle: plugins/markdown/coreAssets Changed:
view changes for bundle: plugins/async-node/coreAssets Changed:
view changes for bundle: plugins/stage-revert-data/coreAssets Changed:
view changes for bundle: plugins/common-types/coreAssets Changed:
view changes for bundle: plugins/check-path/coreAssets Changed:
view changes for bundle: react/playerAssets Changed:
view changes for bundle: plugins/common-expressions/coreAssets Changed:
view changes for bundle: plugins/beacon/coreAssets Changed:
view changes for bundle: plugins/pubsub/coreAssets Changed:
|
KVSRoyal
reviewed
Mar 20, 2026
Comment on lines
+33
to
+48
| /** | ||
| The JS context in which this TinyPubSub instance lives. | ||
| Setting this property loads the PubSubPlugin native bundle (if not already loaded) and | ||
| constructs the JS TinyPubSub instance. Subsequent assignments are ignored once the instance | ||
| has been created. | ||
| */ | ||
| public var context: JSContext? { | ||
| didSet { | ||
| guard let jsContext = context, jsValue == nil else { return } | ||
| if let url = ResourceUtilities.urlForFile(name: "PubSubPlugin.native", ext: "js", bundle: Bundle.module), | ||
| let jsString = try? String(contentsOf: url, encoding: .utf8) { | ||
| jsContext.evaluateScript(jsString) | ||
| } | ||
| jsValue = jsContext.evaluateScript("new PubSubPlugin.TinyPubSub()") | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Can this be moved to the init? Since this is only supposed to happen once, re: "Subsequent assignments are ignored once the instance has been created.", the init seems like the more appropriate place for this to live. Using didSet like this is strange and will confuse users, imo.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## player-1-dot-zero #828 +/- ##
====================================================
- Coverage 85.90% 0 -85.91%
====================================================
Files 507 0 -507
Lines 22891 0 -22891
Branches 2656 0 -2656
====================================================
- Hits 19664 0 -19664
+ Misses 2898 0 -2898
+ Partials 329 0 -329 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
spentacular
reviewed
Mar 24, 2026
spentacular
approved these changes
Mar 24, 2026
sugarmanz
requested changes
Mar 26, 2026
| * sharedBus.subscribe("myEvent") { event, data -> /* ... */ } | ||
| * ``` | ||
| */ | ||
| public class TinyPubSub { |
Member
There was a problem hiding this comment.
Why do we need this exposed for mobile?
Member
Author
There was a problem hiding this comment.
If you wanted to instantiate your own pubsub to pass it in, wouldn't you need a native way to create that object?
Comment on lines
+20
to
+34
| val pubSubVarName = sharedPubSub?.getOrCreate(runtime) | ||
|
|
||
| instance = when { | ||
| pubSubVarName != null && config != null -> { | ||
| runtime.add("pubsubConfig", config) | ||
| runtime.buildInstance("(new $name({...pubsubConfig, pubsub: $pubSubVarName}))") | ||
| } | ||
| pubSubVarName != null -> | ||
| runtime.buildInstance("(new $name({pubsub: $pubSubVarName}))") | ||
| config != null -> { | ||
| runtime.add("pubsubConfig", config) | ||
| runtime.buildInstance("(new $name(pubsubConfig))") | ||
| } | ||
| else -> runtime.buildInstance() | ||
| } |
Member
There was a problem hiding this comment.
I think the TS layer should handle creating the TS things it needs if we don't pass it in. We should interface with as little core internals as possible for the mobile wrappers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #785
Change Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?