Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Helper to bypass mock FS & expose real files/directories #304
Helper to bypass mock FS & expose real files/directories #304
Changes from 14 commits
26bfdf7
be7bf43
a39712e
266569b
95d3ff2
34a4503
91ab56a
fdeed02
0ad9857
c3eddcb
decb5de
135ec12
61b8ac1
b195d8e
3b58f79
6950d39
9f284af
562c6ae
2ca7d0e
10f61da
92ca9c8
172e698
9087580
0c07e57
8e11bec
4b579c0
8dfdc85
2cc92f3
0c7bbbb
f431af2
768224f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate all that you've put together here, @nonara.
I have some concern about the size of the change to the API (adding
bypass
,enable
,disable
,mapPaths
,mapFile
, andmapDir
). And I think the subtle difference in usage ofmapPaths
vsmapFile
ormapDir
is error prone.It feels like we could try to minimize the changes to the API while still providing most of the same functionality.
All of
mapPaths
,mapFile
, andmapDir
look to be about providing a convenient way to load up a mock filesystem with data from the real filesystem. Words likeload
,clone
, andread
come to mind for function names.That implies adding a single
load
function that takes either a path to a directory or a path to a file and copies contents and metadata into a corresponding mock filesystem entry. I assume we should also make it work for symlinks.If we find that usage patterns make it very awkward to repeat the real path in the mock path, we could add a function that makes that more convenient later.
The
bypass
,enable
, anddisable
functions feel like they could be boiled down to a single function that works with either an async/promise returning function or a sync function. In the docs below, you warn against using async calls with the mock disabled - that same warning could be included in thebypass
docs without adding theenable
anddisable
functions.Having a single
bypass
function also gets around the issue that callingenable
twice makesdisable
not work (this is fixable, but could also be avoided by not adding these functions at all).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that
mapDir
andmapFile
could be merged, andload
is a great name.As for mapPaths, this is a simple way to quickly load multiple real paths. (though we can rename -
loadPaths
ormount
?)Bear in mind that while reduce is ugly and difficult to read, this is the shortest solution. Most people will end up using multi-line for loops, etc, all to accomplish what many have requested - a feature to simply 'mount' real areas. Worse yet, some will end up doing
Tedious.
The decision lies with you. I really have spent more time on this than I am able, so I'll go with whatever you want, but I hope you'll consider it reasonable to allow the API to have three new items:
load
,bypass
, andloadPaths
Otherwise, many (myself included) will end up replicating helper logic to convert an array of real locations for each package we use this in.
Agreed. The consideration was that many people will simply use the function without noticing the warning. It was set up to be more strict to prevent people using it and filing issues.
But this is entirely your call, if you're good with it, then it's no problem. I'll add async support, and we can add the warning to both the readme and JSDoc for the types package to make sure it's seen.
Didn't see that. I was up quite late last night. I'll correct the issue. If you're alright with it, I'd like to leave them attached to exports and simply not document in readme or in the @types package. That way there is zero-footprint, but people who know the source can use them.
I can always replicate the behaviour to do it, but this way if anything changes my code doesn't break.
Let me know! Hopefully we can get this wrapped up. I'll wait until your responses before updating anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel tediousness is an issue. I cannot imagine people use mock-fs to load lots of real dirs, it seems defeating the purpose of mocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all. If we have multiple assets directories or even hard-coded paths on the system, this allows
mock-fs
to be used in a way that lets us 'mount' those areas and do whatever we like without actually modifying them. This is tremendously useful with a broad range of use-cases.That is, in fact, the reason I started this PR, but that's really all I can say on that. I don't want to belabor it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good use case.