Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Fallback straight to JS if realpath.native/realpathSync.native throw an exception #36

Closed
wants to merge 4 commits into from

Conversation

rvaidya
Copy link

@rvaidya rvaidya commented Feb 7, 2019

if fs.realpath.native and fs.realpathSync.native crash, it could be because we're on a weird configuration like a RAMdisk on Windows. if so, fall straight back to default JS implementation.

Otherwise, the process.binding attempt may cause the node runtime to abort

I also refactored the code to reduce repetition. Let me know if that is ok.

I was unable to run test.js - getting errors doing npm install coming from "babel-helper-regex" and then getting "Cannot find module 'eslint-config-simenb-node'" when trying to run it anyways.

I tested by copying my updated index.js into my project folder under jest-config and running it that way.

This also fixes a potential typo: both realpath and realpathSync were calling realpath in the process.binding attempt, which was probably unintentional.

…ecause we're on a weird configuration like a RAMdisk on Windows. if so, fall straight back to default JS implementation.
@rvaidya
Copy link
Author

rvaidya commented Feb 7, 2019

#35

@rvaidya
Copy link
Author

rvaidya commented Feb 7, 2019

Getting issues from commitlint, but those can probably be addressed when you squash the commit.

@SimenB
Copy link
Owner

SimenB commented Feb 14, 2019

Thanks for the PR!

This also fixes a potential typo: both realpath and realpathSync were calling realpath in the process.binding attempt, which was probably unintentional.

No, that's intentional - there's no process.binding('fs').realpathSync - the function is always sync. Could you fix that?

$ node -p "Object.keys(process.binding('fs')).sort()"
[ 'FSReqWrap',
  'FileHandle',
  'StatWatcher',
  'access',
  'bigintStatValues',
  'chmod',
  'chown',
  'close',
  'copyFile',
  'fchmod',
  'fchown',
  'fdatasync',
  'fstat',
  'fsync',
  'ftruncate',
  'futimes',
  'internalModuleReadJSON',
  'internalModuleStat',
  'kFsStatsFieldsLength',
  'kUsePromises',
  'lchown',
  'link',
  'lstat',
  'mkdir',
  'mkdtemp',
  'open',
  'openFileHandle',
  'read',
  'readdir',
  'readlink',
  'realpath',
  'rename',
  'rmdir',
  'stat',
  'statValues',
  'symlink',
  'unlink',
  'utimes',
  'writeBuffer',
  'writeBuffers',
  'writeString' ]

Getting issues from commitlint, but those can probably be addressed when you squash the commit.

Yup, don't worry about those 🙂

@rvaidya
Copy link
Author

rvaidya commented Feb 14, 2019

updated

@rvaidya
Copy link
Author

rvaidya commented Apr 7, 2019

Sorry for not getting to this for so long.

I'm still not finding a real clean solution for this - since the async versions are using promises, the only way to handle errors in the first call is to chain promises. If if (typeof fs.realpath.native === 'function') is false, however, the fsBinding approach will still cause the runtime to abort. No great way to preemptively detect the specific Windows + RAMdisk situation and handle it across the board.

I have a callback chaining implementation at https://github.com/rvaidya/realpath-native/tree/feature/windowsRamdiskTry2

This commit doesn't try to refactor anything

@SimenB
Copy link
Owner

SimenB commented Apr 9, 2019

@rvaidya your solution in the linked branch seems OK to me, even though it turned out to be a bit of code. Wanna update this PR to that approach?

@rvaidya
Copy link
Author

rvaidya commented Apr 9, 2019

I opened #39 - you can close this PR

@SimenB
Copy link
Owner

SimenB commented May 7, 2020

I've published a v3 and deprecated it - all current releases of node supports fs.realpath.native now, so this module is no longer needed.

Jest 26 no longer depends on this module.

@SimenB SimenB closed this May 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants