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

assert: make a small internal assert module for built-in use #25956

Closed
wants to merge 3 commits into from
Closed

assert: make a small internal assert module for built-in use #25956

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 6, 2019

First commit:

assert: refactor internal assert.js

Move lib/internal/assert.js to lib/internal/assert/assertion_error.js.
This is in preparation for making lib/internal/assert.js a tiny module
for use in Node.js built-ins so that we can use `assert()` without
having to load the entire ~1200 line `assert` module.

Second commit:

assert: create internal/assert micro-module

For use in built-in modules that could benefit from `assert()` without
having to load the entire module (unless an AssertionError actually
occurs): lib/internal/assert.js.

Third commit:

lib: replace 'assert' with 'internal/assert' for many built-ins

Replace large 'assert' module with tiny 'internal/assert' module for
many built-in uses.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Trott added 3 commits February 5, 2019 20:46
@Trott
assert: refactor internal assert.js
e966998 
Move lib/internal/assert.js to lib/internal/assert/assertion_error.js.
This is in preparation for making lib/internal/assert.js a tiny module
for use in Node.js built-ins so that we can use `assert()` without
having to load the entire ~1200 line `assert` module.
@Trott
assert: create internal/assert micro-module
0e9b3e7 
For use in built-in modules that could benefit from `assert()` without
having to load the entire module (unless an AssertionError actually
occurs): lib/internal/assert.js.
@Trott
lib: replace 'assert' with 'internal/assert' for many built-ins
0a28cdb 
Replace large 'assert' module with tiny 'internal/assert' module for
many built-in uses.
@nodejs-github-bot
Copy link
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2522/pipeline

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 6, 2019
@Trott
Copy link
Member Author

Trott commented Feb 6, 2019 •
edited
Loading

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

BridgeAR
BridgeAR approved these changes Feb 6, 2019
View reviewed changes
test/parallel/test-internal-assert.js

assert.throws(() => { internalAssert(false); }, assert.AssertionError);
assert.throws(() => { internalAssert(false, 'fhqwhgads'); },
{ code: 'ERR_ASSERTION', message: 'fhqwhgads' });
Copy link
Member

Choose a reason for hiding this comment

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

Hint: it is possible to check for the name field as well. That is a good alternative to checking for instanceof assert.AssertionError above (but it's off course not the same strict check).

@Trott
Copy link
Member Author

Trott commented Feb 6, 2019

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20616/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2019
@Trott
Copy link
Member Author

Trott commented Feb 8, 2019

Landed in 7182aca...62942e9

@Trott Trott closed this Feb 8, 2019
Trott added a commit to Trott/io.js that referenced this pull request Feb 8, 2019
@Trott
assert: refactor internal assert.js
64745c3 
Move lib/internal/assert.js to lib/internal/assert/assertion_error.js.
This is in preparation for making lib/internal/assert.js a tiny module
for use in Node.js built-ins so that we can use `assert()` without
having to load the entire ~1200 line `assert` module.

PR-URL: nodejs#25956
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Feb 8, 2019
@Trott
assert: create internal/assert micro-module
5d609bb 
For use in built-in modules that could benefit from `assert()` without
having to load the entire module (unless an AssertionError actually
occurs): lib/internal/assert.js.

PR-URL: nodejs#25956
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Feb 8, 2019
@Trott
lib: replace 'assert' with 'internal/assert' for many built-ins
62942e9 
Replace large 'assert' module with tiny 'internal/assert' module for
many built-in uses.

PR-URL: nodejs#25956
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@Trott @addaleax
assert: refactor internal assert.js
37d207c 
Move lib/internal/assert.js to lib/internal/assert/assertion_error.js.
This is in preparation for making lib/internal/assert.js a tiny module
for use in Node.js built-ins so that we can use `assert()` without
having to load the entire ~1200 line `assert` module.

PR-URL: #25956
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@Trott @addaleax
assert: create internal/assert micro-module
0b4055e 
For use in built-in modules that could benefit from `assert()` without
having to load the entire module (unless an AssertionError actually
occurs): lib/internal/assert.js.

PR-URL: #25956
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Feb 10, 2019
Closed
targos pushed a commit that referenced this pull request Feb 10, 2019
@Trott @targos
lib: replace 'assert' with 'internal/assert' for many built-ins
f40e0fc 
Replace large 'assert' module with tiny 'internal/assert' module for
many built-in uses.

PR-URL: #25956
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@Trott Trott deleted the small-internal-assert branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@JungMinu JungMinu JungMinu 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. lib / src Issues 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.
4 participants
@Trott @nodejs-github-bot @JungMinu @BridgeAR