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

tools: move webcrypto into no-restricted-properties #53023

Merged
merged 1 commit into from
May 30, 2024
Merged

tools: move webcrypto into no-restricted-properties #53023

merged 1 commit into from
May 30, 2024

Conversation

zeeqko
Copy link
Contributor

@zeeqko zeeqko commented May 16, 2024 •
edited
Loading

Since eslint fixed eslint/eslint#16412 in v8.56.0 here and we are on eslint v8.57.0 so that we can take advantage of no-restricted-properties rule for webcrypto.

This is my first pull request to node.js. Please let me know if there is anything to fix.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels May 16, 2024
aduh95
aduh95 reviewed May 16, 2024
View reviewed changes
.eslintrc.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed May 16, 2024
View reviewed changes
test/parallel/test-global-webcrypto.js Outdated
@@ -7,7 +7,7 @@ if (!common.hasCrypto)
const assert = require('assert');
const crypto = require('crypto');

/* eslint-disable no-restricted-syntax */
/* eslint-disable no-restricted-properties, no-restricted-syntax */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to disable no-restricted-syntax?

Suggested change
/* eslint-disable no-restricted-properties, no-restricted-syntax */
/* eslint-disable no-restricted-properties */

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the no-restricted-syntax error comes from test/.eslintrc.yaml here I think the no-restricted-properties should also be added into test/.eslintrc.yaml in order to keep the rule consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding no-restricted-properties into test/.eslintrc.yaml will result in more linting errors in other test files. I have added a TODO which I will try to fix in a separate PR.

@zeeqko zeeqko force-pushed the eslint-rule-update-no-restricted-properties branch from ef00f15 to d34b551 Compare May 18, 2024 06:34
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 23, 2024
@aduh95 aduh95 mentioned this pull request May 23, 2024
Merged
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented May 23, 2024

Can you rebase please? With the move to flat config, there are some conflicts to solve.

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2024
@zeeqko zeeqko force-pushed the eslint-rule-update-no-restricted-properties branch from d34b551 to 40fe042 Compare May 26, 2024 10:18
@zeeqko
Copy link
Contributor Author

zeeqko commented May 26, 2024

Can you rebase please? With the move to flat config, there are some conflicts to solve.

Hi @aduh95, I have rebased my branch. Could you please take a look again and rerun CI for me? Thanks.

I have also removed my TODO as the upgrade of Eslint from v8 to v9 does not cause errors in the test files anymore.

@aduh95
Copy link
Contributor

aduh95 commented May 26, 2024

The linter is failing (also it looks like the TODO is still there?)

@zeeqko
tools: move webcrypto into no-restricted-properties
55cd7e8 
Since eslint fixed eslint/eslint#16412 and we
are on eslint v8.57.0 so that we can take advantage of
no-restricted-properties rule for webcrypto.
@zeeqko zeeqko force-pushed the eslint-rule-update-no-restricted-properties branch from 40fe042 to 55cd7e8 Compare May 26, 2024 11:03
@zeeqko
Copy link
Contributor Author

zeeqko commented May 26, 2024

The linter is failing (also it looks like the TODO is still there?)

Oh sorry. I accidentally dropped one of my commit during interactive rebase. Thank you for spotting it! It should be fixed by now.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@zeeqko
Copy link
Contributor Author

zeeqko commented May 30, 2024

Hi @aduh95, I am still seeing 4 failing CI jobs (feels like flaky tests) could you help me take a look, please? Thanks a lot ! 👀

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 30, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 30, 2024
@nodejs-github-bot nodejs-github-bot merged commit 74dff83 into nodejs:main May 30, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 74dff83

targos pushed a commit that referenced this pull request Jun 1, 2024
@zeeqko @targos
tools: move webcrypto into no-restricted-properties
86e80dc 
Since eslint fixed eslint/eslint#16412 and we
are on eslint v8.57.0 so that we can take advantage of
no-restricted-properties rule for webcrypto.

PR-URL: #53023
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jun 7, 2024
Merged
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
@zeeqko @EliphazBouye
tools: move webcrypto into no-restricted-properties
8f0725b 
Since eslint fixed eslint/eslint#16412 and we
are on eslint v8.57.0 so that we can take advantage of
no-restricted-properties rule for webcrypto.

PR-URL: nodejs#53023
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@zeeqko @bmeck
tools: move webcrypto into no-restricted-properties
85e0daa 
Since eslint fixed eslint/eslint#16412 and we
are on eslint v8.57.0 so that we can take advantage of
no-restricted-properties rule for webcrypto.

PR-URL: nodejs#53023
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakecastelli jakecastelli jakecastelli left review comments

@aduh95 aduh95 aduh95 approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: no-restricted-properties misses destructuring object returned from function call
6 participants
@zeeqko @nodejs-github-bot @aduh95 @lpinca @jakecastelli @targos