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

Use fs.realpathSync.native if available #5031

Merged
merged 3 commits into from
Dec 7, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 6, 2017

Summary
Fixes #5030

Node 9.2 includes nodejs/node#15776, which added more strict input checking (in addition to adding the API we really should be using). Passing in the encoding we want fixes the crash.

Test plan
Manual verification of node 9.2.

@SimenB
Copy link
Member Author

SimenB commented Dec 6, 2017

The 2 usages in Jest were added in #4730 and #5000, so they are just present in the beta releases.

@cjihrig
Copy link

cjihrig commented Dec 6, 2017

I would strongly encourage not using process.binding() directly. There are no guarantees on stability provided.

@SimenB
Copy link
Member Author

SimenB commented Dec 6, 2017

Yeah 🙁

We could check for existence of fs.realpathSync.native and use that if present, I suppose.

@cjihrig
Copy link

cjihrig commented Dec 6, 2017

Can you not try fs.realpathSync.native() and fall back to fs.realpathSync() if it doesn't exist?

@codecov-io
Copy link

codecov-io commented Dec 6, 2017

Codecov Report

Merging #5031 into master will decrease coverage by 0.35%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5031      +/-   ##
==========================================
- Coverage   60.73%   60.37%   -0.36%     
==========================================
  Files         197      198       +1     
  Lines        6598     6640      +42     
  Branches        4        3       -1     
==========================================
+ Hits         4007     4009       +2     
- Misses       2591     2631      +40
Impacted Files Coverage Δ
packages/jest-runtime/src/script_transformer.js 86.82% <100%> (ø) ⬆️
packages/jest-util/src/index.js 94.44% <80%> (+6.2%) ⬆️
packages/jest-mock/src/index.js 84.47% <0%> (-0.37%) ⬇️
...ages/jest-worker/src/__performance_tests__/test.js 0% <0%> (ø)
packages/jest-snapshot/src/plugins.js 100% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 158ecd3...227f334. Read the comment docs.

@SimenB
Copy link
Member Author

SimenB commented Dec 6, 2017

Can you not try fs.realpathSync.native() and fall back to fs.realpathSync() if it doesn't exist?

I've now added usage of fs.realpathSync.native(), but apparently fs.realpathSync() is not enough - see #4730 (comment). So I still fall back to process.bindings

@SimenB SimenB changed the title Pass in encoding to realpath call Use fs.realpathSync.native if available Dec 6, 2017
@cpojer cpojer merged commit 357a119 into jestjs:master Dec 7, 2017
@cpojer
Copy link
Member

cpojer commented Dec 7, 2017

Thanks for fixing this. I'm gonna merge it because the use of process.bindings, while not ideal, wasn't introduced in this diff and therefore is a separate discussion. Obviously we don't want to use something that is not encouraged to use but it seems like we need a different solution from node to remove it.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nodejs 9.2 failure
5 participants