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

Rename internalModuleReadFile to internalModuleReadJSON. #17084

Closed
wants to merge 1 commit into from
Closed

Rename internalModuleReadFile to internalModuleReadJSON. #17084

wants to merge 1 commit into from

Conversation

jdalton
Copy link
Member

@jdalton jdalton commented Nov 16, 2017 •
edited
Loading

This PR addresses #17076 by simply renaming internalModuleReadFile to internalModuleReadJSON.

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).

I'm actually cool with keeping internalModuleReadFile just for the potential to use it for other things besides json, but this was the easy way to address the root of #17076 without digging into c++.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. labels Nov 16, 2017
jdalton
jdalton commented Nov 16, 2017
View reviewed changes
test/parallel/test-module-binding.js
strictEqual(internalModuleReadFile(fixtures.path('empty-with-bom.txt')), '');
strictEqual(internalModuleReadJSON('nosuchfile'), undefined);
strictEqual(internalModuleReadJSON(fixtures.path('empty.txt')), '');
strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt')), '');
{
Copy link
Member Author

@jdalton jdalton Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

👆 These tests highlight the mixed signals being sent:
They test .txt, but the method is being shaped via #15767 to be json-only.

This was referenced Nov 16, 2017
Closed
Closed
@Trott
Copy link
Member

Trott commented Nov 17, 2017

Out of caution, labeling this semver-major since it changes the name of something available via process.binding('fs').

@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 17, 2017
@Trott
Copy link
Member

Trott commented Nov 17, 2017

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

@jdalton
Copy link
Member Author

jdalton commented Nov 17, 2017 •
edited
Loading

Out of caution, labeling this semver-major since it changes the name of something available via process.binding('fs').

Thanks @Trott! Yep, it is as #15767 is (totally semver-major).

@jdalton
Copy link
Member Author

jdalton commented Nov 18, 2017

@Trott does the test fail look real?

@Trott
Copy link
Member

Trott commented Nov 18, 2017

@Trott does the test fail look real?

Not to me, but just in case, here's a rerun of that subset of hosts types:

https://ci.nodejs.org/job/node-test-commit-linux-linked/137/

@jdalton
Copy link
Member Author

jdalton commented Nov 21, 2017

What's the next steps for this PR?

@Trott
Copy link
Member

Trott commented Nov 21, 2017

CI is good. Because it's a breaking change, this will need two approvals from TSC folks. @nodejs/tsc

@Trott
Copy link
Member

Trott commented Nov 21, 2017

Oh, it will also need a CITGM run.

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1097/

(CITGM will no doubt report failures because it always does. Someone will need to go through the results carefully to see if the failures are related or not.)

@Trott
Copy link
Member

Trott commented Nov 21, 2017 •
edited
Loading

Although persuadable, I currently am neutral on this change. I like making the name more accurate, but I don't know that the value of the change outweighs the potential breakage to the ecosystem. Then again, we certainly discourage process.binding() usage, so a breaking change involving that is certainly not as significant as a breaking change in a documented popular API. ¯\(ツ)/¯

@jdalton
Copy link
Member Author

jdalton commented Nov 21, 2017 •
edited
Loading

(CITGM will no doubt report failures because it always does. Someone will need to go through the results carefully to see if the failures are related or not.)

For the CITGM run it may be helpful to run it against the changeset introduced in #15767 too.

Then again, we certainly discourage process.binding() usage, so a breaking change involving that is certainly not as significant as a breaking change in a documented popular API. ¯(ツ)/¯

Testing for the existence of something is easier than testing for the subtle breakage of something. In this case, the changeset of #15767 subtly broke process.bindings("fs").internalReadFile so a clear break in name is easier to detect.

Trott
Trott approved these changes Nov 22, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI and CITGM are clean

@refack
Copy link
Contributor

refack commented Nov 22, 2017

I'll look at the CitGM report in the morning.

@refack
Copy link
Contributor

refack commented Nov 22, 2017

CitGM is clean (nothing new), except lodash-v4.17.4 😱
JK, it's just an installation fail on the Windows machine.

@Trott
Copy link
Member

Trott commented Nov 23, 2017 •
edited
Loading

So, technically, this can land. (Semver major requires two TSC approvals, clean CITGM, clean CI, open for more than 72 hours. We have all those boxes checked.)

@nodejs/tsc and anyone else interested: If you object to this, say something! I'm personally inclined to leave this open through the U.S. holiday weekend and then land it early next week, but if someone wants to land it sooner than that, I won't complain.

@jdalton
Copy link
Member Author

jdalton commented Nov 28, 2017

Ping!

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 28, 2017
@jdalton @Trott
module: rename internalModuleReadFile to internalModuleReadJSON
fea1e05 
PR-URL: nodejs#17084
Fixes: nodejs#17076
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 28, 2017

Landed in fea1e05.

@Trott Trott closed this Nov 28, 2017
@jdalton jdalton deleted the internal-module-read-file branch November 28, 2017 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jdalton @Trott @refack @jasnell @nodejs-github-bot