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 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?
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?

Back to Bug 1875848 Comment 1