Skip to content

src: use node:moduleName as builtin module filename#35498

Closed
targos wants to merge 2 commits into
nodejs:masterfrom
targos:module-filename
Closed

src: use node:moduleName as builtin module filename#35498
targos wants to merge 2 commits into
nodejs:masterfrom
targos:module-filename

Conversation

@targos
Copy link
Copy Markdown
Member

@targos targos commented Oct 4, 2020

This change allows for easier recognition of builtin modules in stack
traces.

Refs: #11893

@targos
src: use node:moduleName as builtin module filename
057d583 
This change allows for easier recognition of builtin modules in stack
traces.

Refs: nodejs#11893
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Oct 4, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Oct 4, 2020

Review requested:

  • @nodejs/startup

@targos targos mentioned this pull request Oct 4, 2020
Open
@targos targos added request-ci Add this label to start a Jenkins CI on a PR. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 4, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Oct 4, 2020 •
edited by targos
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/33379/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2492/

ExE-Boss
ExE-Boss reviewed Oct 4, 2020
View reviewed changes
Comment thread lib/internal/util/inspect.js Outdated
aduh95
aduh95 approved these changes Oct 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I like this change. Maybe we could use primordials here?

Comment thread lib/assert.js Outdated
Comment thread lib/assert.js Outdated
@targos
Copy link
Copy Markdown
Member Author

targos commented Oct 6, 2020

The only new failure in CITGM is with tape: The library doesn't seem directly affected. Two of its tests are broken, like this one: https://github.com/substack/tape/blob/11b7d850ffaa8cb679419ff4a0e314b06a3225b9/test/async-await.js#L182-L186
IMO this kind of breakage is acceptable. The tests would also have to be updated if we moved around functions to different internal files.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Oct 6, 2020

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Oct 7, 2020

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

targos added a commit that referenced this pull request Oct 7, 2020
@targos
src: use node:moduleName as builtin module filename
2178227 
This change allows for easier recognition of builtin modules in stack
traces.

Refs: #11893

PR-URL: #35498
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@targos
Copy link
Copy Markdown
Member Author

targos commented Oct 7, 2020

Landed in 2178227

@targos targos closed this Oct 7, 2020
@targos targos deleted the module-filename branch October 7, 2020 15:35
@targos
Copy link
Copy Markdown
Member Author

targos commented Oct 7, 2020

Forgot to /cc @nodejs/tsc

@mmarchini
Copy link
Copy Markdown
Contributor

mmarchini commented Oct 7, 2020

Semver-major retroactively lgtm

@aduh95 aduh95 mentioned this pull request Oct 14, 2020
Closed
@BethGriggs BethGriggs mentioned this pull request Oct 14, 2020
Merged
@bcoe bcoe mentioned this pull request Oct 15, 2020
Merged
4 tasks
@joyeecheung joyeecheung added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 19, 2020
BethGriggs added a commit that referenced this pull request Oct 19, 2020
@BethGriggs