opral / inlang-sdk

0 stars 0 forks source link

change `variant.match: Array<string>` to `variant.match: Record<string, string>` #111

Open samuelstroschein opened 3 months ago

samuelstroschein commented 3 months ago

Context

Variants with a positional match value will lead broken matches that can't be fixed with lint rules.

Variants have a match array. The order of the values in the match array determines how variants are matched. Unfortunately, the matching order depends on the messages selector order.

Any change in the message's selection order needs to update all variants (💥). There is no guarantee that applications, developers, or users will remember to update the order of all variants. Lint rules are not able to fix the ordering without a history API. Any match value could be correctly positioned.

Message

{
  id: ...,
  locale: ...,
  declarations: ...,
  selectors: [
    {
      type: "expression",
      arg: {
        type: "variable",
        name: "numProducts",
      },
      annotation: {},
    },
    {
      type: "expression",
      arg: {
        type: "variable",
        name: "username",
      },
      annotation: {},
    },
  ]
}

Variant

{
  id: ...
  // is zero matching for username or numProducts?
  match: ["zero", "many"],
  pattern: ...
};

Proposal

Turn match into a Record<string, string> and eliminate the necessity to keep the positioning of message.selectors and variant.match in sync.

{
  id: ...
-  match: ["zero", "many"],
+  match: { username: "many", numProducts: "zero"  }
  pattern: ...
};

The danger of a matching key not being defined in message.selectors or variant.match can be mitigated with a lint rule:

"The matching key 'username' does not exist in the message selector" -> `fix([removeMatchFromVariant, createSelector])`

samuelstroschein commented 3 months ago

came up in a chat with @martin.lysk1

CleanShot 2024-07-07 at 18.16.43@2x.png CleanShot 2024-07-07 at 18.17.06@2x.png
martin-lysk commented 3 months ago

This one is a rabbit whole 🙁 And needs more research:

Good readings:
https://github.com/aphillips/message-format-wg/blob/main/exploration/selection-matching-options.md

Still trying to understand the reasoning behind this one better:
https://github.com/aphillips/message-format-wg/blob/main/spec/formatting.md#example-2

Seems like the order of variants withinin a message also is taken into account here - got to check if we need to take this into account somehow:
https://github.com/aphillips/message-format-wg/blob/main/spec/formatting.md#sort-variants

Some more readings - that may help:

https://github.com/unicode-org/message-format-wg/blob/4de2684f4caa52cafca2d80116f9dd24a513bf89/meetings/2023/notes-2023-03-13.md?plain=1#L111

https://github.com/unicode-org/message-format-wg/issues/351

https://github.com/aphillips/message-format-wg/blob/main/exploration/selection-matching-options.md

Why this is important?

If we change like suggested:

{
  id: ...
-  match: ["zero", "many"],
+  match: { likes: "many", shares: "zero"  }
  pattern: ...
};

This changes - "changes" on an atom variant since we loose the order information on match - that was there before. an alternative would be:

{
  id: ...
-  match: ["zero", "many"],
+  match: [{shares: "zero" }, { likes: "many" }]
  pattern: ...
};

Lets play through the possible scenarios based on those variants:

Current model

[zero, many] -> variant A
[zero, *]    -> variant B
[*,    *]    -> variant C

Proposed model A

{ likes: zero, numProducts: many } -> variant A
{ likes: zero, numProducts: * }    -> variant B
{ likes: *, numProducts: * }    -> variant C

Proposed model B - with sorting info by array position

[{ likes: zero},{ numProducts: many }] -> variant A
[{ likes: zero},{ numProducts: * }]    -> variant B
[{ likes: *},{ numProducts: many }]    -> variant C

Proposed model C - with selectors having an identifier (not in the examples)

{
  selectors: [
    {"id": "8675c28c-24f9-4b82-b26d-7ece28a395e8", "name": "numProducts" },
    {"id": "0ca50bb6-e329-45fb-9818-0df6440daf44", "name": "likes" },
  ] 
}
[{ id: "8675c28c-24f9-4b82-b26d-7ece28a395e8", match: zero},{ id: "0ca50bb6-e329-45fb-9818-0df6440daf4", match: many }] -> variant A
[{ id: "8675c28c-24f9-4b82-b26d-7ece28a395e8", match: zero},{ id: "0ca50bb6-e329-45fb-9818-0df6440daf4", match: * }]    -> variant B
[{ id: "8675c28c-24f9-4b82-b26d-7ece28a395e8", match: *},{ id: "0ca50bb6-e329-45fb-9818-0df6440daf4", match: many }]    -> variant C

Selector Inserted:

[likes, numProducts] -> [gender, userName, numProducts]

Current model

we cant match matchers to selectors afterwards -> match of all variants need to be updated in one transaction - not possible via linting:

[*, zero, many] -> variant A
[*, zero, *]    -> variant B
[*, *,    *]    -> variant C

Proposed model

Linting Possible - no direct action on each variant needed:

{ likes: zero, numProducts: many } -> variant A
{ likes: zero, numProducts: * }    -> variant B
{ likes: *, numProducts: * }    -> variant C

After lints are fixed - all messages receive a change

{ gender: *, likes: zero, numProducts: many } -> variant A
{ gender: *, likes: zero, numProducts: * }    -> variant B
{ gender: *, likes: *, numProducts: * }    -> variant C

Same for Proposed model B

[{ gender: *},{ likes: zero},{ numProducts: many }] -> variant A
[{ gender: *},{ likes: zero},{ numProducts: * }]    -> variant B
[{ gender: *},{ likes: *},{ numProducts: many }]    -> variant C

Selector Renamed:

[likes, numProducts] -> [numLikes, numProducts]

[zero, many] -> variant A
[zero, *]    -> variant B
[*,    *]    -> variant C

no change the variants. That is a nice property - since it doesn't change the variant - its just a name of a selector.

Proposed model A

Lint triggers - nonSelectorMatchingMatchers

{ likes: zero, numProducts: many } -> variant A - LINT triggers
{ likes: zero, numProducts: * }    -> variant B - LINT triggers
{ likes: *, numProducts: * }    -> variant C.   - LINT triggers

Consequence Variant matcher names need to be editable in ui?

{ numLikes: zero, numProducts: many } -> variant A - LINT triggers
{ numLikes: zero, numProducts: * }    -> variant B - LINT triggers
{ numLikes: *, numProducts: * }    -> variant C.   - LINT triggers

Same for Proposed model B

Selector Moved:

[likes, numProducts] -> [numProducts, likes]

Current Model

All variants matchers need to be updated:

[many, zero] -> variant A
[*, zero]    -> variant B
[*,    *]    -> variant C

Proposed model A

- variants stay the same - since order is not part of the model - no change on variant

Prosed model B

Lint triggers - match-order does not match selector-order

Fix would need to update all variants to:

[{ numProducts: many },{ likes: zero}] -> variant A
[{ numProducts: * },{ likes: zero},]    -> variant B
[{ numProducts: many },{ likes: *}]    -> variant C

Question for me here:

Do we want to enforce a change on variants on selector move (makes sense when it impacts the variants selection priority!?) - this would speak for Model B.

Do we wan't to enforce a change on a variant in case of selector renaming? If not - we would need to consider Model C where selectors get an Id.

martin-lysk commented 3 months ago

@nils.jacobsen this also impacts the bundle component!

NilsJacobsen commented 3 months ago

@martin.lysk1

we cant match matchers to selectors afterwards -> match of all variants need to be updated in one transaction - not possible via linting:

That seems to be the main probelm right? Can you explain a bit more what causes the issue in detail?

martin-lysk commented 3 months ago

In the current approach we don't have a reference to the selectors other than order in the matchers. As soon as the list with respect to the position of its elements change (this happens on moving selectors, inserting, and deleting (not renaming)) you loose the connection between a selector and a matcher within the match array of the variant.

This means you have to update all variants in the same transaction as the message(with the new selectors).

In a distributed system this can only be enforced if we define a Message as an atom and expect those cases to be dealt with during merge.

NilsJacobsen commented 3 months ago

I thought a message is already a atom?

martin-lysk commented 3 months ago

Nope - that was how it was modelled in the rxdb/slot storage setup - now we have a varaint table

NilsJacobsen commented 3 months ago

Ahh we bundle and variant now? Or just variants?

martin-lysk commented 3 months ago

We have three tables in sqlite aka three atoms:

Bundles -> Messages -> Variants

https://github.com/opral/monorepo/blob/martinlysk1/mesdk-154-main/inlang/source-code/sdk-v2/src/newProjectOpfs.ts#L50

NilsJacobsen commented 3 months ago

let's sync in around

samuelstroschein commented 3 months ago

@martin.lysk1 thanks for the detailed write up. as discussed on call, i would choose option B.

{
  id: ...
+ match: { username: "many", numProducts: "zero" }
  pattern: ...
};
  1. taking positioning out of matchers makes variants simpler and leads to no change if the position changes.

  2. "atomicy" is not a concern of inlang but lix. Lix needs to work irrespective of how apps define their data structures. Otherwise, lix is not generalizable. Lix must be able to raise a merge conflict if "actor A added a new variant and actor B changed the matching order on a message".

NilsJacobsen commented 3 months ago

Isn't the problem that, since we don't have a gatekeeper that allows us to do an addSelector mutation (update message, updateVariants, insertVariants), we need to process that user interaction in multiple transactions? That means Lix might raise a conflict while processing these transactions. Even though they get resolved after the last one.

Maybe I got it wrong :D

samuelstroschein commented 3 months ago

I don't understand what you mean with transaction. Loom maybe?

NilsJacobsen commented 3 months ago

Little update: https://www.loom.com/share/e62f523855c1437b82d13b27f7349be3

TL;DR:

samuelstroschein commented 3 months ago

@nils.jacobsen any problem that arises should be fixable by lint rules. if a matcher does not exist for a variant, there -should- will be a lint rule that detects the error

martin-lysk commented 3 months ago

I sketched down the cases that we could face with one variant.

We will need to be able to visualize those cases in the UI somehow and provide a mechanism for each - case.

  1. A new selector "newSelector" has been added to the message which the variant does have yet.
    1. Lint fix -> add missing matcher
    2. ui -> show an empty field for the matcher in the variant
  2. The name of the selector has changed for "newName",
    1. Lint fix -> ?
    2. ui -> add a column for "oldName" selector?
  3. The Variant contains a matcher that the message does not know.
    1. Lint fix 1 -> add selector (we only have the name of the selector not the type, arg etc)
    2. Lint fix 2 -> remove the matcher without selector from the variant
    3. UI -> add a column for "nonExistingSelector" selector?
{
    "id": "keen_next_deer_charm",
    "alias": {
        "default": "name_steep_direct_walrus_surge"
    },
    "messages": [
        {
            "id": "09e31f50-a3d7-47cd-b9e2-1b44231b5ab1",
            "bundleId": "keen_next_deer_charm",
            "locale": "en",
            ...
            "selectors": [
                {
                    "type": "expression",
                    "arg": {
                        "type": "variable",
                        "name": "newName"
                    },
                    ...
                },
                {
                    "type": "expression",
                    "arg": {
                        "type": "variable",
                        "name": "newSelector"
                    },
                    ...
                },

            ],
            "variants": [
                {
                    "id": "c3ee4b64-80d5-4a40-95df-376ef54b7ca8",
                    "match": {
                        "oldName": "other",
                        "nonExistingSelector": "other"
                    },
                    "pattern": [
                        {
                            "type": "text",
                            "value": "Welcome user!"
                        }
                    ]
                },

            ]
        }
    ]
} 
samuelstroschein commented 3 months ago

We will need to be able to visualize those cases in the UI somehow and provide a mechanism for each - case.

Lix inlang plugin will have a differ 😎

martin-lysk commented 3 months ago

No further action required for now