feat(runner): call sync directly when not in shim; allow URIs in .sync calls #1463
Conversation
…c calls - Update storage.ts to call sync directly when not running in a shim environment. - Allow either EntityId or URIs on sync call (eventually we'll deprecate the former)
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the storage sync functionality by allowing direct sync calls when not in a shim environment and expanding the sync method to accept URIs in addition to EntityIds. The changes prepare for future deprecation of EntityId in favor of URI-based identifiers.
- Direct sync calls when not running in shim environment to improve performance
- Updated sync method signatures to accept both EntityId and URI types
- Added URI validation logic in the BaseStorageProvider's toEntity method
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/runner/src/storage/interface.ts | Updated sync method signature to accept EntityId or URI |
| packages/runner/src/storage/cache.ts | Updated sync method signatures in ProviderConnection and Provider classes |
| packages/runner/src/storage/base.ts | Enhanced toEntity method to handle string URIs with validation |
| packages/runner/src/storage.ts | Added direct sync call path when not in shim environment |
Comments suppressed due to low confidence (1)
packages/runner/src/storage/base.ts:87
- [nitpick] The error message uses an emoji which may not display properly in all environments and could make logs harder to parse programmatically. Consider using a standard error prefix.
`💣 Got entity ID that is a string, but not a URI: ${source}`,
There was a problem hiding this comment.
cubic analysis
No issues found across 4 files. Review in cubic
packages/runner/src/storage.ts
Outdated
| if (!this.shim) { | ||
| const { space, id } = cell.getAsNormalizedFullLink(); | ||
| const storageProvider = this._getStorageProviderForSpace(space); | ||
| return storageProvider.sync( |
There was a problem hiding this comment.
since this is in an async function, I'd rather have:
await storageProvider.sync(id, false, schemaContext);
return cell;
packages/runner/src/storage/cache.ts
Outdated
|
|
||
| sync( | ||
| entityId: EntityId, | ||
| entityId: EntityId | Entity, |
There was a problem hiding this comment.
this should probably be URI instead of Entity for clarity, and we should do the same below (line 1445)
There was a problem hiding this comment.
URI is an alias to Entity in the other file, but not here, not sure why.
|
I ended up folding my suggestions, as well as the more substantial move away from EntityId into #1464, so this should probably just land as-is. |
|
Ah, I think your change now includes all relevant bits of this one. Closing. |
* Minor tweaks - Use await instead of a promise chain in Storage.syncCell. - Use URI isntead of Entity, since we also define this in memory/interface.ts. * Change IStorageProvider API to be URI based instead of EntityId as well as the BaseStorageProvider implementation and the Storage class. Moved BaseStorageProvider.toEntity into Storage.toURI, since this is the only place it's needed. Changed IStorageProvider to use URI instead of EntityId, as well as the ProviderConnection and Provider classes. Updated tests to use cell.getAsNormalizedFullLink to get the URI instead of EntityId.
Summary by cubic
Updated storage to call sync directly when not running in a shim and added support for URIs in sync calls.