Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

Support mkdir at mountpoints #1120

Merged
merged 5 commits into from
Apr 8, 2021
Merged

Conversation

phated
Copy link
Contributor

@phated phated commented Apr 6, 2021

I'm working on a CLI tool that bundles some code, but then mirrors the directory structure out to the filesystem. This is done using mountpoints and this patch that allows fs.mkdir to apply to them.

function mkdirInSnapshot(path_, cb) {
var cb2 = cb || rethrow;
return cb2(
new Error('Cannot mkdir in a snapshot. Try mountpoints instead.')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better error for here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead change the name of the method in something more user friendly, something like mkdirThrowInSnapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with mkdirFailInSnapshot because it doesn't throw if a callback is provided to the async method, but it still fails.

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Could you add a test for this?

@phated
Copy link
Contributor Author

phated commented Apr 6, 2021

Test added! I named it with the PR number. Let me know if it should be something different.

@phated
Copy link
Contributor Author

phated commented Apr 6, 2021

Hmm, this had the same pnpm failure as #1121

@robertsLando
Copy link
Contributor

robertsLando commented Apr 7, 2021

Seems that the problem is node version, strange them were not failing before... @erossignon could you fix those tests please?

@erossignon
Copy link
Contributor

Let me provide a patch for the pnpm issue...
We will skip the pnpm test when nodejs 10 is detected.

@erossignon
Copy link
Contributor

please merge #1122 first

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsLando
Copy link
Contributor

This can be merged once check pass

@robertsLando
Copy link
Contributor

@leerob can we merge this?

@leerob leerob merged commit 8756fc1 into vercel:master Apr 8, 2021
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.

None yet

4 participants