fix: set currentSpace inside addSpace if currentSpace isn't set #575
Conversation
I noticed this wasn't happening because w3ui's w3console was in a state where the space picker was populated with many spaces, but currentSpace wasn't set. I think we should avoid this situation at this level because it's most convenient for me, but open to moving this assurance to another level if.
| * @param {import('./types').SpaceMeta} meta | ||
| * @param {import('@ucanto/interface').Delegation} [proof] | ||
| */ | ||
| async addSpace(did, meta, proof) { | ||
| this.spaces.set(did, meta) | ||
| if (!this.currentSpace) { |
There was a problem hiding this comment.
It's not obvious to me that calling addSpace would also have the side effect of changing this.currentSpace, either from the comments or tests.
Why should this have that side effect, instead of leaving this to the caller to decide whether or not to call Agent#setCurrentSpace? The latter seems like the most flexible solution.
There was a problem hiding this comment.
(and, even if it is desirable to introduce the side effect here, why would this do it via assigning this.currentSpace = vs calling this.setCurrentSpace?)
| @@ -111,12 +111,15 @@ export class AgentData { | |||
|
|
|||
| /** | |||
| * @deprecated | |||
| * @param {import('@ucanto/interface').DID} did | |||
| * @param {import('@ucanto/interface').DID<'key'>} did | |||
There was a problem hiding this comment.
It could be useful to indicate which did (space). (e.g. it might be ambiguous considering the proof @param itself is composed of several DIDs)
| @@ -754,7 +754,7 @@ export async function addSpacesFromDelegations(access, delegations) { | |||
|
|
|||
| for (const [did, value] of Object.entries(allows)) { | |||
| if (did.startsWith('did:key') && value['space/*']) { | |||
| data.addSpace(/** @type {Ucanto.DID} */ (did), { | |||
| data.addSpace(/** @type {Ucanto.DID<'key'>} */ (did), { | |||
| * @param {import('./types').SpaceMeta} meta | ||
| * @param {import('@ucanto/interface').Delegation} [proof] | ||
| */ | ||
| async addSpace(did, meta, proof) { | ||
| this.spaces.set(did, meta) | ||
| if (!this.currentSpace) { | ||
| this.currentSpace = did |
There was a problem hiding this comment.
Side effects make me nervous. I'd feel better about it if the agent class called setCurrent after adding a space(s)...at this level there's no choice for anyone using the data class.
Please if you decide not to move then can you at least add a comment to the method in the agent class to say that it will set current space if one is not set? 🙏
|
eh, if both of you feel like adding this here is too icky I'm not gonna do it - will find another place to do this! |
|
FWIW I think you made the right decision 😘 |
|
thanksssss me too, thx to both of you for pushing this in the right direction! |
🤖 I have created a release *beep* *boop* --- ## [6.0.0](storacha/w3ui@solid-keyring-v5.0.1...solid-keyring-v6.0.0) (2023-11-21) ### ⚠ BREAKING CHANGES * use w3up-client in keyring ([storacha#581](storacha/w3ui#581)) ### Features * add support for getting an account's plan ([storacha#564](storacha/w3ui#564)) ([11023a4](storacha/w3ui@11023a4)) * re-export Service from `react-keyring` ([storacha#577](storacha/w3ui#577)) ([308816d](storacha/w3ui@308816d)) * use w3up-client in keyring ([storacha#581](storacha/w3ui#581)) ([bd5f341](storacha/w3ui@bd5f341)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Travis Vachon <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [6.0.0](storacha/w3ui@solid-keyring-v5.0.1...solid-keyring-v6.0.0) (2023-11-21) ### ⚠ BREAKING CHANGES * use w3up-client in keyring ([storacha#581](storacha/w3ui#581)) ### Features * add support for getting an account's plan ([storacha#564](storacha/w3ui#564)) ([9565452](storacha/w3ui@9565452)) * re-export Service from `react-keyring` ([storacha#577](storacha/w3ui#577)) ([94e8b96](storacha/w3ui@94e8b96)) * use w3up-client in keyring ([storacha#581](storacha/w3ui#581)) ([2edddd9](storacha/w3ui@2edddd9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Travis Vachon <[email protected]>
I noticed this wasn't happening because w3ui's w3console was in a state where the space picker was populated with many spaces, but currentSpace wasn't set.
I think we should avoid this situation at this level because it's most convenient for me, but open to moving this assurance to another level if.