New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[labs/analyzer] Fix support for static class members. #3648
Conversation
🦋 Changeset detectedLatest commit: 5531665 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about splitting the four getters into separate ones for static vs. member? I get the feeling that it's more likely that a user would care about only one group at a time since they represent properties of different objects, which might avoid extra (e.g.) classDecl.fields.filter(x => !x.isStatic)
.
} | ||
|
||
/** | ||
* Returns iterator of the `ClassMethod`s defined on the immediate class | ||
* (excluding any inherited members). | ||
*/ | ||
get methods(): IterableIterator<ClassMethod> { | ||
return this._methodMap.values(); | ||
get methods() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does removing this return type cause it to implicitly be Array<ClassMethod>
? Is that something we care about w.r.t. API stability / flexibility? (vs. the original return type, which is more generic?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, and I don't think we care that much about stability at this exact point. It's not possible to make a generator getter directly (where we could just yield both values() out; it would need to call out to one), and didn't figure it was worth the trouble to keep this an iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, separating them allows keeping that return type I guess. Might as well.
Yeah, that's a good point. It's likely a documentation generator wants to ensure the static and non-static members are collated, so it'd need to do just that I think. I'll separate them. |
/** | ||
* Returns a `ClassField` model the given name defined on the immediate class | ||
* (excluding any inherited members). | ||
*/ | ||
getField(name: string): ClassField | undefined { | ||
return this._fieldMap.get(name); | ||
getField(name: string, isStatic = false): ClassField | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have fields
and staticFields
, I'm not sure what value there is in a parameter to separate the two here. I would default to mirroring the two APIs and making getStaticField()
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sgtm, done
We were previously storing class members (fields & methods) in a map by name (since field-lookup by name is expected to be a key use case), however this ignored the fact that static and non-static members would conflict.
This PR separates the storage into separate maps, and adds an
isStatic
optional argument togetField()
andgetMethod()
.