It turns out the problem here was actually that uniffi allows a POJO - ie, we are calling, roughly: > setTab({url: "http://...", icon: "" }) instead of: > setTab(new RemoteTabRecord("http://...", "")) The generated code works in both cases (any type with the correct property names will work) - but because only the constructor handles default values. IMO, it's fairly clear that the first example above should fail with a TypeError, forcing the second (right?) BUT - this still isn't ideal when considering defaults (or even records with many fields). Consider suggest - it ends up generating: ``` export class RemoteSettingsConfig { constructor(serverUrl = null,bucketName = null,collectionName) { ``` which is really quite useless for defaults - the only way the default values here could be leveraged would be to call it like `new RemoteSettingsConfig(undefined, undefined, "the-collection")` which seems very unergonomic. An alternative would be a breaking change where instead we generate: ``` export class RemoteSettingsConfig { constructor({serverUrl = null, bucketName = null, collectionName}) { ``` which would mean the consumer can call `new RemoteSettingsConfig({ collectionName: "the-collection" })` but the downside is that *all* consumers of *all* structs must use this object syntax when constructing the record. The upsides are that args can be specified in any order, all arguments are self-documenting (no guessing what that 5th string param is!) and default values become saner and also self-documenting. This will be a breaking change for suggest and quite possibly tabs/sync15, but it should be easy to fix and I'm leaning towards this being the right thing to do but I'm really not sure. Ben, Lina, thoughts?
Bug 1875848 Comment 1 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
It turns out the problem here was actually that uniffi allows a POJO - ie, we are calling, roughly: > setTab({url: "http://...", icon: "" }) instead of: > setTab(new RemoteTabRecord("http://...", "")) The generated code works in both cases (any type with the correct property names will work) - but fails because only the constructor handles default values. IMO, it's fairly clear that the first example above should fail with a TypeError, forcing the second (right?) BUT - this still isn't ideal when considering defaults (or even records with many fields). Consider suggest - it ends up generating: ``` export class RemoteSettingsConfig { constructor(serverUrl = null,bucketName = null,collectionName) { ``` which is really quite useless for defaults - the only way the default values here could be leveraged would be to call it like `new RemoteSettingsConfig(undefined, undefined, "the-collection")` which seems very unergonomic. An alternative would be a breaking change where instead we generate: ``` export class RemoteSettingsConfig { constructor({serverUrl = null, bucketName = null, collectionName}) { ``` which would mean the consumer can call `new RemoteSettingsConfig({ collectionName: "the-collection" })` but the downside is that *all* consumers of *all* structs must use this object syntax when constructing the record. The upsides are that args can be specified in any order, all arguments are self-documenting (no guessing what that 5th string param is!) and default values become saner and also self-documenting. This will be a breaking change for suggest and quite possibly tabs/sync15, but it should be easy to fix and I'm leaning towards this being the right thing to do but I'm really not sure. Ben, Lina, thoughts?