Conversation
940e933 to
ae73d80
Compare
src/mixins/form-associated.js
Outdated
|
|
||
| delegate({ | ||
| properties: this[formAssociated].properties, | ||
| delegate.call(Class, { |
There was a problem hiding this comment.
Inside delegate(), we use this in the getters we define, so we need Class as a context.
There was a problem hiding this comment.
We should fix delegate then, that’s a utility method, not a static method
| }; | ||
|
|
||
| if (sourceDescriptor.writable || sourceDescriptor.set) { | ||
| if (sourceDescriptor?.writable || sourceDescriptor?.set) { |
There was a problem hiding this comment.
Some sourceDescriptors can be undefined, e.g., the ones that are not in ElementInternals.prototype (e.g., type).
There was a problem hiding this comment.
Yes, it's one of the default properties:
There was a problem hiding this comment.
Er, that's not a real property (yet). No idea why it's there.
| let descriptor = conflictPolicy.canMerge(prop) ? getMergeDescriptor(targetDescriptor, sourceDescriptor) : sourceDescriptor; | ||
| Object.defineProperty(target, prop, descriptor); |
There was a problem hiding this comment.
If I understand correctly, if we don't skip or throw or if prop is not in target, we should merge, so this code should be outside if.
| for (let prop of [ | ||
| ...Object.keys(sourceDescriptors), | ||
| ...Object.getOwnPropertySymbols(sourceDescriptors), | ||
| ]) { |
There was a problem hiding this comment.
Otherwise, we miss symbols.
There was a problem hiding this comment.
getOwnPropertyDescriptors should return symbols too (unlike getOwnPropertyNames)
There was a problem hiding this comment.
It does, but for…in doesn't iterate over them
There was a problem hiding this comment.
Ah, true, for .. in skips symbols. Reflect.ownKeys() should give you all of them.
|
|
||
| function getMergeDescriptor (targetDescriptor, sourceDescriptor) { | ||
| if (!canMerge(targetDescriptor, sourceDescriptor)) { | ||
| if (!targetDescriptor || !canMerge(targetDescriptor, sourceDescriptor)) { |
There was a problem hiding this comment.
Don't throw on undefined targetDescriptior.
| let Super = Object.getPrototypeOf(this); | ||
|
|
||
| if (Super[self]) { | ||
| if (!Super[self]) { |
There was a problem hiding this comment.
Was this based on the comment? Because the logic seems right to me
There was a problem hiding this comment.
Not only on the comment. Without the change, the global styles defined on a super will not be handled. For example, if we define a color element, the global styles defined on the parent ColorElement class are ignored. But I might miss some nuances of the main idea of the refactored code, though. 🤔
There was a problem hiding this comment.
The idea is that if a class doesn't have [self] then it doesn't have this mixin, and any globalStyles properties it may have are incidental. Does ColorElement have this property?
There was a problem hiding this comment.
Okay, now I got it. So, to make the GlobalStyles mixin work, a class should import the self symbol from global.js and add a static property [self] with any (?) value (I added static [self] = self). Is that correct?
There was a problem hiding this comment.
No, applying the mixin should add it automatically.
There was a problem hiding this comment.
Well, this isn't happening. I'll take a closer look
| let config = this.constructor[formAssociated] ?? this.constructor.formAssociated; | ||
| let { like, role, valueProp, changeEvent } = config; | ||
| let internals = this[internals] || this.attachInternals(); | ||
| this[internals] ??= this.attachInternals(); |
There was a problem hiding this comment.
This should already be done inside attachInternals, is it not?
There was a problem hiding this comment.
This covers cases where this.attachInternals was already called before [constructed]() is executed, e.g., in a class constructor (like we do in ColorElement).
There was a problem hiding this comment.
It has nothing to do with [constructed]. We override attachInternals() in L44. Since mixins are applied before instances are constructed, that should work.
src/mixins/form-associated.js
Outdated
|
|
||
| static [onApply] () { | ||
| let config = this[formAssociated] || this.formAssociated; | ||
| static [onApply] (Class) { |
There was a problem hiding this comment.
Why? This is a static method, so this should resolve to the class anyway
There was a problem hiding this comment.
I've found the "real" issue—we called it incorrectly: we should've passed Class as a context, not a parameter. Fixed
There was a problem hiding this comment.
Where did you fix it? I see this PR is closed.
There was a problem hiding this comment.
Here (you've already moved my change to your PR):
Co-Authored-By: Dmitry Sharabin <dmitrysharabin@gmail.com>
No description provided.