-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
visitorKeys is needed for prettier #1467
Conversation
|
||
QUnit.module('[glimmer-syntax] Public API is unchanged'); | ||
|
||
QUnit.test('exports are not accidentally removed', function(assert) { |
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.
if there is a better way to do this, I'd be all ears. I think it'd be great to have some sort of snapshot tool to at least automatically manage Object.keys(syntax)
packages/@glimmer/syntax/index.ts
Outdated
@@ -32,6 +32,8 @@ export type { NodeVisitor } from './lib/traversal/visitor'; | |||
export { default as Walker } from './lib/traversal/walker'; | |||
export * as ASTv1 from './lib/v1/api'; | |||
export { default as builders } from './lib/v1/public-builders'; | |||
export { default as visitorKeys } from './lib/v1/visitor-keys'; | |||
export { voidMap } from './lib/generation/printer'; |
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.
Since voidMap
is generated from array, can we export the array instead?
voidTagNames.split(' ').forEach((tagName) => { |
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.
I also suggest switch to use Set
internally.
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.
yeah, I'm a fan of this -- one less module-load-time thing to do. Gonna type out this array, and not have this runtime code.
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.
I also suggest switch to use Set internally.
good thinkin -- we're still cleaning up from back when Set
wasn't available everywhere
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.
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.
You made a mistake
- const voidTagSet = new Set(...voidTagArray);
+ const voidTagSet = new Set(voidTagArray);
yes the object access is very fast, but not much differences.
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.
tyty, all this is generally hotpath code, so every little bit counts!
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.
Will this also fix #1436 ? |
yes! |
…t vs Map's perf differences are not important enough right now (since this is build-time code), and maintainability takes precedence in this case
Use a Set for checking if an element is a void element, which is faster than object access
voidTagNames.split(' ').forEach((tagName) => { | ||
voidMap[tagName] = true; | ||
}); | ||
export const voidMap = new Set([ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: fisker Cheung <lionkay@gmail.com>
Resolves: #1465 and #1436