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

fix(core): guard usages of performance.mark #52505

Closed

Conversation

alan-agius4
Copy link
Contributor

While performance.mark is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

This commit, updates the usage to a safer variant.

@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Nov 3, 2023
@ngbot ngbot bot added this to the Backlog milestone Nov 3, 2023
@alan-agius4 alan-agius4 changed the title fix(@angular/core): guard usages of performance.mark fix(core): guard usages of performance.mark Nov 3, 2023
@alan-agius4 alan-agius4 added area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate labels Nov 3, 2023
@alan-agius4 alan-agius4 modified the milestones: Backlog, v17-final Nov 3, 2023
@alan-agius4 alan-agius4 marked this pull request as ready for review November 3, 2023 11:49
@alan-agius4 alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 3, 2023
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this pull request Nov 3, 2023
@alan-agius4 alan-agius4 requested review from devversion and removed request for AndrewKushnir and josephperrott November 3, 2023 12:45
alan-agius4 added a commit to angular/angular-cli that referenced this pull request Nov 3, 2023
More context in angular/angular#52505

(cherry picked from commit 5f14bf3)
alan-agius4 added a commit to angular/angular-cli that referenced this pull request Nov 3, 2023
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM overall, but not sure about the lint rule

packages/core/src/util/performance.ts Show resolved Hide resolved
tools/tslint/noPerformanceRule.ts Outdated Show resolved Hide resolved
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

This commit, updates the usage to a safer variant.
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-for: global-approvers

packages/core/src/util/performance.ts Outdated Show resolved Hide resolved
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.
@alan-agius4 alan-agius4 removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 3, 2023
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Nov 3, 2023
@atscott
Copy link
Contributor

atscott commented Nov 3, 2023

This PR was merged into the repository by commit d4bd3f1.

@atscott atscott closed this in 93d32a9 Nov 3, 2023
atscott pushed a commit that referenced this pull request Nov 3, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

PR Close #52505
atscott pushed a commit that referenced this pull request Nov 3, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

This commit, updates the usage to a safer variant.

PR Close #52505
atscott pushed a commit that referenced this pull request Nov 3, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

PR Close #52505
@alan-agius4 alan-agius4 deleted the performance-mark-guarded branch November 3, 2023 14:44
markName: string,
markOptions?: PerformanceMarkOptions|undefined,
): PerformanceMark|undefined {
return performance?.mark?.(markName, markOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late comment. I'm wondering if we should make it even safer by checking if the performance is defined as a global first, i.e.:

return typeof performance !== 'undefined' ? performance.mark?.(markName, markOptions) : undefined;

@alan-agius4 WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far I didn’t encountered any occurrences that requires such check.

Abseil-byte pushed a commit to Abseil-byte/angular that referenced this pull request Nov 5, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

This commit, updates the usage to a safer variant.

PR Close angular#52505
Abseil-byte pushed a commit to Abseil-byte/angular that referenced this pull request Nov 5, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

PR Close angular#52505
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 4, 2023
tbondwilkinson pushed a commit to tbondwilkinson/angular that referenced this pull request Dec 6, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

This commit, updates the usage to a safer variant.

PR Close angular#52505
tbondwilkinson pushed a commit to tbondwilkinson/angular that referenced this pull request Dec 6, 2023
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

PR Close angular#52505
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

This commit, updates the usage to a safer variant.

PR Close angular#52505
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
While `performance.mark` is available on all supported browsers and node.js version this API is not available in JSDOM which is used by Jest and Cloudflare worker.

PR Close angular#52505
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants