Skip to content
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

perf - do not use class for Codicon #171234

Closed
wants to merge 2 commits into from
Closed

perf - do not use class for Codicon #171234

wants to merge 2 commits into from

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Jan 13, 2023

This speeds up codicons.js module load time 100% for me:

Before
Screenshot 2023-01-13 at 09 48 57

After
Screenshot 2023-01-13 at 09 48 12

@bpasero bpasero enabled auto-merge (squash) January 13, 2023 08:49
@bpasero bpasero requested a review from aeschli January 13, 2023 08:50
@bpasero bpasero self-assigned this Jan 13, 2023
@bpasero bpasero added this to the January 2023 milestone Jan 13, 2023
@@ -13,582 +13,607 @@ export function getCodiconAriaLabel(text: string | undefined) {
return text.replace(/\$\((.*?)\)/g, (_match, codiconName) => ` ${codiconName} `).trim();
}

export interface ICodicon extends CSSIcon {
readonly codiconFontCharacter: string;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeschli fyi I decided here to name fontCharacter actually codiconFontCharacter to be able to implement a good isCodicon method that can check on that. Previously we were able to do instanceof Codicon, but with this change we no longer can. If we kept fontCharacter, there is a chance we would wrongly match on an object that has both id and fontCharacter. Since I found a few more types with fontCharacter I decided to go this path, but please verify that is OK.

@bpasero
Copy link
Member Author

bpasero commented Jan 13, 2023

Hm, I wonder if we can even get rid of namespace and use something along the lines of:

export const Codicon = {

	// built-in icons, with image name
	'add': makeCodicon('add', '\\ea60'),

Because looking at microsoft/TypeScript#51387, there seems to be additional lookup cost when using namespace. The only then to keep in mind is that you cannot reference another Codicon from within that object literal as we do for some icons ('treeItemExpanded': makeCodicon('tree-item-expanded', chevronDown.codiconFontCharacter))

@bpasero
Copy link
Member Author

bpasero commented Jan 13, 2023

Better done in #171254

@bpasero bpasero closed this Jan 13, 2023
auto-merge was automatically disabled January 13, 2023 13:56

Pull request was closed

@bpasero bpasero deleted the ben/proud-snake branch January 19, 2023 09:58
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant