-
Notifications
You must be signed in to change notification settings - Fork 23
feat: createContexts #368
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
feat: createContexts #368
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 877f4ed | Commit Preview URL Branch Preview URL |
Nov 04 2025, 12:32 AM |
hugomrdias
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can this be more simple like:
user knows what he wants
type ProviderDatasetPair {
providerId: bigint
datasetId: bigint | undefined // create new dataset in this provider
}
createContexts(pairs: ProviderDatasetPair[], options: createDatasetOptions)smart selection
type Options {
excludeProviderIds?: number []
withCDN?: boolean
withIpni?: boolean
dev?: boolean
forceCreateDataSet?: boolean
uploadBatchSize?: number
metadata?: Record<string, string>
}
createSmartContexts(options: Options) // its auto and smart| for (const dataSetId of new Set(options.dataSetIds)) { | ||
| const resolution = await StorageContext.resolveByDataSetId( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just force options?.dataSetIds.length to equal options.count ?
| for (const dataSetId of new Set(options.dataSetIds)) { | ||
| const resolution = await StorageContext.resolveByDataSetId( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
humm because its selecting providers...
This isn't our behavior for |
sure we can the same and not force the create dataset if its undefined |
I've thought about this a bit, and while the priority is more implicit, I think the interface is slightly better without this intermediate createContexts({
dataSetIds,
})
createContexts({
providerIds,
})
// vs ProviderDatasetPair
createContexts({
selections: dataSetIds.map((dataSetId) => { dataSetId }),
})
createContexts({
selections: providerIds.map((providerId) => { providerId }),
})I would also be interested in @rvagg's opinion on this. It's not too important to me but after we make this interface public it will be painful to change it.
I don't think we need multiple methods since both will fallback to smart selection. It's sufficient for me that the specification parameters are optional. |
Co-authored-by: Hugo Dias <hugomrdias@gmail.com>
|
Sorry, I'm not a fan of
That second path I think is the most interesting one and the one we want to make sure works - perhaps data set IDs get persisted in a cookie for a dapp user, every time they come back they always just get those same data sets that were created for them the first time they used it. Every other bit of optionality is advanced use I think. |
|
I'm fine with override-and-fix since we have release-please standing in the way of releases. If @hugomrdias would like to propose an alternative diff then let's get that in before we cut a release. Sorry Hugo, let's keep moving and fix stuff later if we discover the API isn't all that great. |

Supercedes #325
Toward #312
Reviewers @rvagg @hugomrdias
As this is not yet the blessed path, I don't update the examples from
createContexttocreateContexts.Changes