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.

Sign up for GitHub

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

Jump to bottom

lib, doc: standardize params in PerformanceObserver.observe #47025

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

lib, doc: standardize params in PerformanceObserver.observe #47025

wants to merge 2 commits into from

Conversation

umuoy1
Copy link
Contributor

@umuoy1 umuoy1 commented Mar 9, 2023

This fixes missing validation when options.buffered and options.entryType are both set in PerformanceObserver.observe
https://www.w3.org/TR/performance-timeline/#observe-method

If options's entryTypes is present and any other member is also present, then throw a "TypeError".

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 9, 2023
@umuoy1 umuoy1 changed the title lib, doc: throw TypeError when options.buffered and options.entryType both set lib, doc: inappropriate params validation in observe Mar 15, 2023
@umuoy1 umuoy1 changed the title lib, doc: inappropriate params validation in observe lib, doc: standardize params in PerformanceObserver.observe Mar 17, 2023
@legendecas legendecas added semver-major PRs that contain breaking changes and should be released in the next major version. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 31, 2023
legendecas
legendecas reviewed Mar 31, 2023
View reviewed changes
test/fixtures/wpt/performance-timeline/po-observe-type.any.js
@@ -33,6 +33,13 @@ test(() => {
});
}, "Calling observe() with type and entryTypes should throw a TypeError");

test(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Changes in WPT should be submitted to the upstream first: https://github.com/web-platform-tests/wpt.

In the meantime, you can move this test case to test/parallel/ directory. That's where Node.js' test cases are located at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have the changes been upstreamed?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50737/

@pavel-romanov8 pavel-romanov8 mentioned this pull request Apr 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@legendecas legendecas legendecas left review comments

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@umuoy1 @nodejs-github-bot @legendecas @aduh95