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
feat: dynamic scoped ids #787
Conversation
Benchmark resultsBase commit: lwc-engine-benchmark
|
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.
This is a very fine work @ekashida! let's roll!
Benchmark resultsBase commit: lwc-engine-benchmark
|
a8d1573
to
c390247
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
@@ -592,6 +592,17 @@ export function k(compilerKey: number, obj: any): number | string | void { | |||
} | |||
|
|||
// [g]lobal [id] function | |||
export function gid(id: string): string { | |||
export function gid(id: any): string | null | undefined { | |||
if (isNull(id) || isUndefined(id)) { |
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.
probably faster to do == null
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.
Sorry for the n00b question, but what when does == null
evaluate to true
?
export function gid(id: string): string { | ||
export function gid(id: any): string | null | undefined { | ||
if (isNull(id) || isUndefined(id)) { | ||
// - Custom elements: `id="null"` and `id="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.
what about the error? assert.logError()
return id; | ||
} | ||
if (isString(id) && !id.length) { | ||
// - Empty strings will render a boolean attribute: `id` |
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.
log error here too
// - Native elements will not render the id attribute | ||
return id; | ||
} | ||
if (isString(id) && !id.length) { |
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.
maybe we can move this into the previous condition, saying (id == null || id === '')
return the same id
export function gid(id: any): string | null | undefined { | ||
if (isNull(id) || isUndefined(id) || id === '') { | ||
if (process.env.NODE_ENV !== 'production') { | ||
assert.logError(`Invalid id value "${id}". Expected a non-empty string.`, vmBeingRendered!.elm); |
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.
Not too sure about this vmBeingRendered!.elm
.
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Does this PR introduce a breaking change?