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

Feature/windows ramdisk try2 #39

Closed
wants to merge 4 commits into from

Conversation

rvaidya
Copy link

@rvaidya rvaidya commented Apr 9, 2019

Not the cleanest of solutions but it works - limiting the check to the specific error message we get from node in this situation.

Could try to deduplicate repeated code in the realpath function but that might hurt readability.

index.js Outdated
@@ -1,3 +1,5 @@
/* eslint-disable prefer-arrow-callback */
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, my auto eslint kept changing my functions into arrow functions so I added that to stop them.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with arrows all over

Copy link
Owner

Choose a reason for hiding this comment

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

But not eslint ignores

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what minimum version of node you want to support but arrow functions were added a bit later.

I've resolved this in the latest commit.

Copy link
Owner

Choose a reason for hiding this comment

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

Node 4 is the lowest :)

@SimenB
Copy link
Owner

SimenB commented Apr 9, 2019

I think some sort of isWinRamDiskError makes sense, which just checks code (and has the comment) makes sense

@rvaidya
Copy link
Author

rvaidya commented Apr 9, 2019

I think some sort of isWinRamDiskError makes sense, which just checks code (and has the comment) makes sense

The problem in the realpath function is you don't know that you've run into the situation up front, as you're immediately returning a promise -- unless you want to add something like let isWinRamDiskError = err.errno === -4068 purely for clarity without changing the control flow.

Ignore this

@rvaidya
Copy link
Author

rvaidya commented Apr 9, 2019

This next commit uses recursion to dedupe the code

@aliaksandr-yermalayeu
Copy link

@rvaidya, is there any progress on this one ?

@aliaksandr-yermalayeu
Copy link

@SimenB, 2 months passed, and there is still no response from @rvaidya. Are you ok if I create another pull request?

@rvaidya
Copy link
Author

rvaidya commented Jun 25, 2019

My last commit works and is still waiting for review, there isn't anything more I can do on my end

@aliaksandr-yermalayeu
Copy link

aliaksandr-yermalayeu commented Jun 25, 2019

Oh, I was under impression you didn't finalize the PR (it still has failing checks). Maybe I was wrong. @SimenB, could you please speed up the process from your side?

index.js Outdated Show resolved Hide resolved
@aliaksandr-yermalayeu
Copy link

@SimenB, any news?

@aliaksandr-yermalayeu
Copy link

@SimenB, @rvaidya, would you take a look this PR?

@sergei-startsev
Copy link

@SimenB Is there a chance to merge this PR? The issue blocks Jest upgrade and we're forced to use version 23. v24 was published 6 month ago and v25 is coming soon, however the issue is still unresolved. I'd appreciate if you could comment it.

@rominator1983
Copy link

What would be neccessary to get this merged? Do you need any help on this? This stops me being able to use a RAM-disk (#35).

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

5 participants