Implement a merge() method to apply partials and mixins #584
Conversation
|
This is pretty far from done, but sending it as a slightly-working checkpoint, in order to comment on a few issues. |
| } | ||
| } | ||
|
|
||
| // merge partials (including partial mixins) |
There was a problem hiding this comment.
The idea here is to first merge partials and then merge mixins, performing basically the same steps each time. But this will have the downside of copying members and merging extended attributes twice in the case of a partial interface mixin. (The mixin step isn't here yet.)
Performance isn't much of a concern, but I wonder if this is overgeneralized, in particular it becomes hard to judge whether https://heycam.github.io/webidl/#dfn-exposure-set is followed.
The alternative would be to first collect all includes statements and then to copy members directly from their source to their final destination.
There was a problem hiding this comment.
in particular it becomes hard to judge whether https://heycam.github.io/webidl/#dfn-exposure-set is followed.
Not sure I follow this part, in what situation would it be problematic?
There was a problem hiding this comment.
I mean if the implementation isn't similar to the algorithm in the spec, it's harder to know if it get the same result in all cases. I think the two-step approach would be equivalent, but I'm not certain. The rules about whether the extended attributes of the member have to be a subset of the container's or if the intersection is used differ at least, so a unified implementation would allow for more invalid stuff.
There was a problem hiding this comment.
Hmm, well, I'd say we can go this way and fix it if it turns out to be problematic.
| if (copyExposure.size !== 1) { | ||
| value = `(${value})`; | ||
| } | ||
| copy.extAttrs = ExtendedAttributes.parse( |
There was a problem hiding this comment.
@saschanaz I found it quite hard to get this right, and I didn't. The code here ultimately needs to copy any extended attributes from partial/mixin to its members, and to merge [Exposed] as part of that. That might end up adding, removing or modifying the original list of extended attributes. It seems like the only way to do this will be to carefully modify the original tokens, serialize it, and then ExtendedAttributes.parse the string. Are there helpers that will make this easier, or should I go about it differently?
There was a problem hiding this comment.
That's something requested by #490, and no good alternative exists. I think this is okay for now until I actually implement it.
| if (copyExposure.size !== 1) { | ||
| value = `(${value})`; | ||
| } | ||
| copy.extAttrs = ExtendedAttributes.parse( |
There was a problem hiding this comment.
That's something requested by #490, and no good alternative exists. I think this is okay for now until I actually implement it.
| * @return {*[]} | ||
| */ | ||
| export function merge(ast) { | ||
| const dfns = new Map(); |
There was a problem hiding this comment.
Probably good to reuse this function:
Line 24 in 49c514f
| } | ||
| } | ||
|
|
||
| // merge partials (including partial mixins) |
There was a problem hiding this comment.
in particular it becomes hard to judge whether https://heycam.github.io/webidl/#dfn-exposure-set is followed.
Not sure I follow this part, in what situation would it be problematic?
| } | ||
|
|
||
| expect.extend({ | ||
| toMergeAs(received, expected) { |
There was a problem hiding this comment.
Wow, didn't know expect supports extension this way 👍
There was a problem hiding this comment.
Me neither, I found out after I had repeated myself a lot and wanted a helper function :)
This patch closes #581 and includes: