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.

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

Rename InternalModuleReadFile and friends. #17076

Closed
jdalton opened this issue Nov 16, 2017 · 2 comments
Closed

Rename InternalModuleReadFile and friends. #17076

jdalton opened this issue Nov 16, 2017 · 2 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem.

Comments

@jdalton
Copy link
Member

jdalton commented Nov 16, 2017

With the addition of PR #15767 the internal InternalModuleReadFile and pseudo exposed process.binding("fs").internalModuleReadFile are no longer generic read file helpers and are essentially locked down to just json. InternalModuleReadFile was more generically useful before and could have been applied to loading more than just json.

I think the name InternalModuleReadFile, and the pseudo exposed process.binding("fs").internalModuleReadFile, should either be renamed or a new internal for json-only should be created (maybe that wraps InternalModuleReadFile).

@bnoordhuis
Copy link
Member

That pull request's been open for six weeks, why didn't you comment then?

I don't think the name matters, it's used in only one place. Pull requests welcome, I guess, but honestly, it seems very low value.

@jdalton
Copy link
Member Author

jdalton commented Nov 16, 2017

That pull request's been open for six weeks, why didn't you comment then?

I wasn't aware of the PR until I saw the commit message next to "lib/module.js" when navigating files on GitHub 😋

I don't think the name matters, it's used in only one place. Pull requests welcome, I guess, but honestly, it seems very low value.

The name and function matter, in part, because it's an appealing pseudo private. Since this is designated a semver major change anyway it might as well be a clean break with either an API name change or a new helper.

I mentioned the potential usefulness of this helper to @Trott in the hallway track of Node Interactive. Do others have an opinion or preference for direction?

Update:
Opened PR #17084.

@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. labels Nov 16, 2017
@Trott Trott closed this as completed in fea1e05 Nov 28, 2017
jack-teng1 added a commit to jack-teng1/electron that referenced this issue Jun 18, 2018
due to change in node.js(nodejs/node#17076)
internalModuleReadFile has been renamed to internalModuleReadJSON, and
in vendor/node/lib/internal/modules/cjs/loader.js it will call
internalModuleReadJSON and result in can't read the package.json file.
jack-teng1 added a commit to jack-teng1/electron that referenced this issue Jun 18, 2018
due to change in node.js(nodejs/node#17076)
internalModuleReadFile has been renamed to internalModuleReadJSON, and
in vendor/node/lib/internal/modules/cjs/loader.js it will call
internalModuleReadJSON and result in can't read the package.json file.
jack-teng1 added a commit to jack-teng1/electron that referenced this issue Jun 19, 2018
due to change in node.js(nodejs/node#17076)
internalModuleReadFile has been renamed to internalModuleReadJSON, and
in vendor/node/lib/internal/modules/cjs/loader.js it will call
internalModuleReadJSON and result in can't read the package.json file.
bergheim added a commit to bergheim/pkg that referenced this issue Jul 5, 2018
As per nodejs/node#17076 the functions have been
renamed in node now. This reenables dynamic loading of modules using the node10
runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants