Skip to content

lib: refactor to use validateObject - #37028

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
Lxxyx:refactor-to-use-validate-object
Jan 28, 2021
Merged

lib: refactor to use validateObject#37028
aduh95 merged 1 commit into
nodejs:masterfrom
Lxxyx:refactor-to-use-validate-object

Conversation

@Lxxyx

@LxxyxLxxyx commented Jan 23, 2021

Copy link
Copy Markdown
Member

No description provided.

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 23, 2021
ExE-Boss
ExE-Boss reviewed Jan 23, 2021
View reviewed changes

@ExE-BossExE-Boss left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These require(…) calls should probably all be multiline:

Comment threadlib/internal/process/per_thread.js Outdated
Comment threadlib/path.js Outdated
Comment threadlib/internal/assert/assertion_error.js Outdated
Comment threadlib/internal/streams/end-of-stream.js Outdated
Comment threadlib/internal/util/inspect.js Outdated
Comment threadlib/repl.js Outdated
Comment threadlib/trace_events.js Outdated
Comment threadlib/perf_hooks.js Outdated
Comment threadlib/internal/url.js Outdated
aduh95
aduh95 approved these changes Jan 23, 2021
View reviewed changes
Comment threadlib/internal/streams/end-of-stream.js Outdated
@Trott

Trott commented Jan 23, 2021

Copy link
Copy Markdown
Member

Most of these represent a change in behavior where an array was previously permitted but now it is not. I don't think that's a problem, but pointing it out in case others think we should be cautious about that and maybe rate this as semver-major.

@TrottTrott mentioned this pull request Jan 23, 2021
Merged
@Lxxyx
Lxxyxforce-pushed the refactor-to-use-validate-object branch from 5964a61 to d777cbcCompareJanuary 24, 2021 06:54
@ExE-BossExE-Boss mentioned this pull request Jan 24, 2021
Merged
@ExE-Boss

ExE-Boss commented Jan 24, 2021

Copy link
Copy Markdown
Contributor

@Trott

Most of these represent a change in behavior where an array was previously permitted but now it is not.

I’m adding an allowArray option to validateObject in #37047 to prevent this from being a breaking change (and also to fix the WebIDL‑based APIs).

@nodejs-github-bot

nodejs-github-bot commented Jan 26, 2021

Copy link
Copy Markdown
Collaborator

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

@LxxyxLxxyx added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2021
Trott added a commit to Trott/io.js that referenced this pull request Jan 27, 2021
@Trott
tools: enable object-curly-newline in ESLint rules
3e54870
I saw a PR review comment about newlines in desructured
assignments, and this would be the rule to enforce these kinds of nits.
Start by just enabling the rule. We can incrementally adjust it to be
more strict.
Refs: https://eslint.org/docs/rules/object-curly-newline
Refs: nodejs#37028 (review)
PR-URL: nodejs#37040
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot

nodejs-github-bot commented Jan 27, 2021

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jan 27, 2021

Copy link
Copy Markdown
Collaborator

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

@Lxxyx
Lxxyxforce-pushed the refactor-to-use-validate-object branch from 14eca5f to 3686855CompareJanuary 28, 2021 05:16
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jan 28, 2021

Copy link
Copy Markdown
Collaborator

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

@Lxxyx@aduh95
lib: refactor to use validateObject
029d1fd
Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#37028
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95
aduh95force-pushed the refactor-to-use-validate-object branch from 3686855 to 029d1fdCompareJanuary 28, 2021 11:55
@aduh95

aduh95 commented Jan 28, 2021

Copy link
Copy Markdown
Contributor

Landed in 029d1fd

@aduh95aduh95 closed this Jan 28, 2021
@aduh95
aduh95 merged commit 029d1fd into nodejs:masterJan 28, 2021
@Lxxyx
Lxxyx deleted the refactor-to-use-validate-object branch January 28, 2021 13:47
targos
targos reviewed Jan 29, 2021
View reviewed changes
Comment threadlib/internal/child_process.js
(options === null || typeof options !== 'object')) {
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
} else if (options !== undefined) {
validateObject(options, 'options');

@targostargosJan 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI, this change broke cheerio through the workerpool module: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2604/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/cheerio_v1_0_0_rc_5/

See https://github.com/josdejong/workerpool/blob/b0e72695fc26df60e224c2d2eafc402f3097a6f4/src/WorkerHandler.js#L262

It would probably be nice to open a PR to fix that module. It seems to have been working by accident until now.

@aduh95aduh95Jan 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done in josdejong/workerpool#230.

@ExE-BossExE-BossJan 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably also do #37047.

targos pushed a commit that referenced this pull request Feb 2, 2021
@Trott@targos
tools: enable object-curly-newline in ESLint rules
d2d6121
I saw a PR review comment about newlines in desructured
assignments, and this would be the rule to enforce these kinds of nits.
Start by just enabling the rule. We can incrementally adjust it to be
more strict.
Refs: https://eslint.org/docs/rules/object-curly-newline
Refs: #37028 (review)
PR-URL: #37040
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Feb 2, 2021
@Lxxyx@targos
lib: refactor to use validateObject
67b58f6
Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #37028
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targostargos mentioned this pull request Feb 2, 2021
Merged
@kanongil

kanongil commented Mar 3, 2021

Copy link
Copy Markdown
Contributor

The workerpool breakage is more serious than fixing the head version. My project uses both v2 and v3 through various dependencies I have little control over.

@Trott

Trott commented Mar 3, 2021

Copy link
Copy Markdown
Member

The workerpool breakage is more serious than fixing the head version. My project uses both v2 and v3 through various dependencies I have little control over.

Ooof. The fix is in workerpool@6.1.0, released about one month ago.

Unfortunately, workerpool@6.1.0 accounts for less than 1/3 of the package's over 2.2 million downloads in the last week.

targos pushed a commit that referenced this pull request May 30, 2021
@Trott@targos
tools: enable object-curly-newline in ESLint rules
2a8db68
I saw a PR review comment about newlines in desructured
assignments, and this would be the rule to enforce these kinds of nits.
Start by just enabling the rule. We can incrementally adjust it to be
more strict.
Refs: https://eslint.org/docs/rules/object-curly-newline
Refs: #37028 (review)
PR-URL: #37040
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
@Trott@targos
tools: enable object-curly-newline in ESLint rules
85d0442
I saw a PR review comment about newlines in desructured
assignments, and this would be the rule to enforce these kinds of nits.
Start by just enabling the rule. We can incrementally adjust it to be
more strict.
Refs: https://eslint.org/docs/rules/object-curly-newline
Refs: #37028 (review)
PR-URL: #37040
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
@Trott@targos
tools: enable object-curly-newline in ESLint rules
cbced2f
I saw a PR review comment about newlines in desructured
assignments, and this would be the rule to enforce these kinds of nits.
Start by just enabling the rule. We can incrementally adjust it to be
more strict.
Refs: https://eslint.org/docs/rules/object-curly-newline
Refs: #37028 (review)
PR-URL: #37040
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
@Trott@targos
tools: enable object-curly-newline in ESLint rules
f868fac
I saw a PR review comment about newlines in desructured
assignments, and this would be the rule to enforce these kinds of nits.
Start by just enabling the rule. We can incrementally adjust it to be
more strict.
Refs: https://eslint.org/docs/rules/object-curly-newline
Refs: #37028 (review)
PR-URL: #37040
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@loqsloqs mentioned this pull request Mar 8, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targostargostargos left review comments
@aduh95aduh95aduh95 approved these changes
@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@RaisinTenRaisinTenRaisinTen approved these changes
+1 more reviewer
@ExE-BossExE-BossExE-Boss left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Lxxyx@Trott@ExE-Boss@nodejs-github-bot@aduh95@kanongil@jasnell@targos@RaisinTen