openscd / open-scd-core

Apache License 2.0
5 stars 8 forks source link

Update Edit API handles namespaced attributes poorly #132

Open ca-d opened 1 year ago

ca-d commented 1 year ago

The Update event could use an update, since the current handling of namespaced attribute updates is both cumbersome and - in one edge case - insufficient.

Currently attributes are grouped by (unprefixed) name and attach an extra namespaceURI parameter to the value:

const update: Update = {
  element,
  attributes: { attrName: { namespaceURI: 'https://example.org/myNS', value: 'attrValue' } }
}

The cumbersome aspect is that namespaceURI is quite long to type.

The functionally insufficient aspect is that this disallows setting two different attributes from two different namespaces if they happen to be named the same, e.g. if I wanted to simultaneously set both the svg:x attribute and the sxy:x attribute from the SVG and IEC 61850-6 Annex C.1 namespaces, respectively.

I suggest changing this part of the API by separating namespaced from non-namespaced attributes in the update event like so:

const updateV2: UpdateV2 = {
  element,
  attributes: { attrName: 'attrValue' } // non-namespaced attrs only
  attributesNS: { 'https://example.org/myNS': { attrName: 'attrValue' } }
}

resulting in a new type

export type UpdateV2 = {
  element: Element;
  attributes: Partial<Record<string, string | null>>;
  attributesNS: Partial<Record<string, Partial<Record<string, string | null>>>>;
};

Since this project is still in alpha, I think the new type could actually replace the current Update type without too much trouble, which seems preferable to me. If this does not happen until the first stable version of open-scd-core is released, I would then tend more to adding a new UpdateV2 type in addition to the old one and consequently supporting the deprecated legacy API til kingdom come.

trusz commented 1 year ago

If I understand correctly an example with more attributes could look like this?

updateV2: UpdateV2 = {
  element,
  attributes: { attrName: 'attrValue' } // non-namespaced attrs only
  attributesNS: { 
    'https://example.org/myNS': { 
       attrName1: 'attrValue1',
       attrName2: 'attrValue2', 
    },
    'https://example.org/userNS': { 
       name: 'John',
       age: '42', 
    }
  }
}

probably one object would look weird right? so just one attributes object and an empty key string would be the non-namespaces attribues

updateV2: UpdateV2 = {
  element,
  attributes: { 
    '': { // non-namespaced attrs only
      attrName: 'attrValue'
    } 
    'https://example.org/myNS': { 
       attrName1: 'attrValue1',
       attrName2: 'attrValue2', 
    },
    'https://example.org/userNS': { 
       name: 'John',
       age: '42', 
    }
  }
}
trusz commented 1 year ago

which alternative names have you considered? e.g.:

ca-d commented 1 year ago

probably one object would look weird right? so just one attributes object and an empty key string would be the non-namespaces attribues

Yeah empty would look weird I think. I liked attributes and attributesNS because it mirrors the Web API's naming with setAttribute and setAttributeNS and is therefore a bit mnemonic for the Web API methods that we actually invoke upon receiving the events in question.

trusz commented 1 year ago

Oh yes. That makes sense!