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

fs: Refactor the import of internalUtil #33296

Closed
wants to merge 1 commit into from
Closed

fs: Refactor the import of internalUtil #33296

wants to merge 1 commit into from

Conversation

rickyes
Copy link
Contributor

@rickyes rickyes commented May 8, 2020 •
edited
Loading

Deconstructing the .deprecate() that introduces internal/util.

/cc @BridgeAR

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label May 8, 2020
@addaleax
Copy link
Member

addaleax commented May 8, 2020

@rickyes This module isn’t going to be loaded unless fs has already been loaded, so lazy-loading seems counterproductive here.

@rickyes
Copy link
Contributor Author

rickyes commented May 8, 2020 •
edited
Loading

@rickyes This module isn’t going to be loaded unless fs has already been loaded, so lazy-loading seems counterproductive here.

@addaleax Thanks for the comment, sorry for the my oversight, I will remove the lazy loading fs changes and keep the deconstruction introduced into internal/util.

@rickyes rickyes force-pushed the refactor-fs-require branch from dd5e196 to 95d58e7 Compare May 8, 2020 03:03
@rickyes rickyes changed the title fs: Refactor the import of fs and internalUtil fs: Refactor the import of internalUtil May 8, 2020
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member

Landed in 1a12b82

addaleax pushed a commit that referenced this pull request May 20, 2020
@rickyes @addaleax
fs: refactor the import of internalUtil
1a12b82 
PR-URL: #33296
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@addaleax addaleax closed this May 20, 2020
@rickyes rickyes deleted the refactor-fs-require branch May 20, 2020 13:06
@codebytere
Copy link
Member

Marking as dont-land for 12.x since this changes code introduced in #29061, which is semver-major.

codebytere pushed a commit that referenced this pull request Jun 18, 2020
@rickyes @codebytere
fs: refactor the import of internalUtil
a90b96f 
PR-URL: #33296
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrHeer MrHeer MrHeer approved these changes

@himself65 himself65 himself65 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax 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. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rickyes @addaleax @nodejs-github-bot @codebytere @joyeecheung @himself65 @MrHeer