Conversation
✅ Deploy Preview for nude-element ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
- Apply mixins on initialization of the class itself and its children - `observedAttributes` getter should have context
LeaVerou
force-pushed
the
class-mixins
branch
from
da5a0bd to
6d3a93e
Compare
Co-Authored-By: Dmitry Sharabin <dmitrysharabin@gmail.com>
- Separate prototype copying from superclasses - Simplify logic - Drop recursive: number and just automatically stop at the first shared superclass ping @DmitrySharabin
Co-Authored-By: Dmitry Sharabin <dmitrysharabin@gmail.com>
- Make it easier to check whether a style was adopted by a given root - add `adoptCSSRecursive()` and use it in the globalStyles mixin
- Simplifies the global styles mixin by removing superclasses traversal and updating style resolution logic. - Refactors fetchCSS to handle promises and failed fetches more robustly. - `fetchCSS()` already uses a cache, so now we're reading it directly on render to get the most updated version @DmitrySharabin can you please check that this works?
Co-Authored-By: Dmitry Sharabin <dmitrysharabin@gmail.com>
| let skippedProperties = new Set(options.skippedProperties || []); | ||
| let sourceDescriptors = Object.getOwnPropertyDescriptors(source); | ||
|
|
||
| for (let prop of Reflect.ownKeys(sourceDescriptors)) { |
There was a problem hiding this comment.
I've tried Reflect.ownKeys() before switching to Object.keys() and Object.getOwnPropertySymbols(), and it seems like it gives us more properties than we get with for...in or iterating over object keys and symbols. For example, it gives us all enumerable properties (not only symbols). Is it okay?
There was a problem hiding this comment.
All own properties is what we need (strings and symbols). I'm not sure if we need non-own properties too, I'll need to think more about that. But we definitely want own ones.
There was a problem hiding this comment.
Yes, own should be enough. We handle superclasses separately in extend-class.js
src/mixins/form-associated.js
Outdated
| properties: [ | ||
| "labels", | ||
| "form", | ||
| "name", |
There was a problem hiding this comment.
We should also remove name. It's not (yet?) in ElementInternals.prototype.
There was a problem hiding this comment.
Ideally, name should be a prop though, since that's how you name form controls. But that can be done later.
src/mixins/form-associated.js
Outdated
| this[internals].setFormValue(this[valueProp])); | ||
| } | ||
|
|
||
| static formAssociated = true; |
There was a problem hiding this comment.
I noticed that this shadows a class's own formAssociated properties when this mixin is in use. That means that in [onApply](), config is an empty object regardless. I might suggest the opposite. I don't know which part of the code should be fixed.
There was a problem hiding this comment.
Ooooh, well spotted. I have an idea: mixins can include their own conflictPolicy through a symbol to indicate which properties should be skipped, merged, etc. This one could be a property that is skipped. But I'll need to do that.
There was a problem hiding this comment.
Actually, specifically for this one, I just had an idea that's easier.
| [constructed] () { | ||
| let config = this.constructor[formAssociated] ?? this.constructor.formAssociated; | ||
| let { like, role, valueProp, changeEvent } = config; | ||
| let internals = this[internals] || this.attachInternals(); |
There was a problem hiding this comment.
Here, we are accessing a variable at the time it is defined. We don't need the internals var anymore; it's been replaced by the internals symbol. Following up on our conversation in the PR, we should probably simply call this.attachInternals() here, no?
There was a problem hiding this comment.
This internals var is used differently: to store the internals object. We should replace its usage with this[internals].
There was a problem hiding this comment.
It's not used anywhere in the code. One place I thought it was, is this one:
But here, we pass it as a prop (“prop name”), not the object itself. That's why I replaced this line with this[internals] ??= this.attachInternals() in my PR (and you mentioned that it was wrong), so IDK what to do with this line. Do we need it at all?
Co-authored-by: Dmitry Sharabin <dmitrysharabin@gmail.com>
Co-authored-by: Dmitry Sharabin <dmitrysharabin@gmail.com>
Co-authored-by: Dmitry Sharabin <dmitrysharabin@gmail.com>
`ConflictPolicy` seems like an overcomplex solution to the problems, but let's see how it goes, we can always hardcode its abstractions later
Updated ConflictPolicy to clarify strategy types, refactored property handling for merge, overwrite, skip, and throw, and improved exception management.
Simplifies the getSuper function to return only the superclass prototype, removing the ability to fetch a specific property. Updates attachInternals to use the new getSuper signature.
Avoids accidentally extending mixin lifecycle hooks, a problem spotted by @DmitrySharabin
Updated ConflictPolicy to support direct exceptions object and refactored extendClass to instantiate ConflictPolicy for both instance and static options.
LeaVerou
force-pushed
the
class-mixins
branch
from
a7cf54b to
af7a105
Compare
2 participants
No description provided.