Skip to content

Conversation

@amcasey
Copy link
Member

@amcasey amcasey commented May 13, 2021

If a project has many of them (e.g. 1800), parsing the patterns repeatedly can take up a lot of time.

Specifically, getOwnKeys and hasZeroOrOneAsteriskCharacter were showing up as self-time hotspots.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 13, 2021
@amcasey amcasey marked this pull request as ready for review May 19, 2021 23:22

export function tryParsePattern(pattern: string): Pattern | undefined {
// This should be verified outside of here and a proper error thrown.
Debug.assert(hasZeroOrOneAsteriskCharacter(pattern));
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove the assert or requirement that users of this have this checked already?

Copy link
Member Author

Choose a reason for hiding this comment

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

The check was duplicated between the caller and the assert (none needed it for other reasons - they were just doing it to satisfy this requirement). I didn't think the requirement made the API conceptually clearer or the implementation simpler - on the contrary, I thought it was preferable to gather all the checks into a single function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps your question was "what happened to the proper error?". It's still there - an undefined result now indicates that an error should be reported.

amcasey added 2 commits May 25, 2021 15:44
If a project has many of them (e.g. 1800), parsing the patterns
repeatedly can take up a lot of time.
@amcasey
Copy link
Member Author

amcasey commented May 25, 2021

Force push is a rebase onto master. Updates to follow.

@amcasey
Copy link
Member Author

amcasey commented May 26, 2021

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 26, 2021

Heya @amcasey, I've started to run the tarball bundle task on this PR at 0435303. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 26, 2021

Hey @amcasey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/103950/artifacts?artifactName=tgz&fileId=5B3FB4B8589F2D1B2AB57D830882879C2CC6FD618829A38998245D6627FCD24802&fileName=/typescript-4.4.0-insiders.20210526.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@amcasey amcasey merged commit 3ffa245 into microsoft:master May 26, 2021
@amcasey amcasey deleted the PathPatterns branch May 26, 2021 16:40
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants