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

feat: added fs built-in module #1752

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

streamich
Copy link

No description provided.

@casr
Copy link

casr commented Nov 15, 2017

Looks cool but wouldn't mocking the fs module in this instance have surprising results? For instance, if I were to try to read a file in my repo, it would need to be included in the bundle so memfs would find the file?

@streamich
Copy link
Author

streamich commented Nov 15, 2017

@casr memfs will find whatever is in memfs, you could pre-package memfs with whatever you need from your repo:

const {vol} = require('memfs');

vol.fromJSON({
  '/package.json': '...'
});

Also, it is not about allowing to read files from your repo, but for packages that expect there to be a file system, they would just work.

@casr
Copy link

casr commented Nov 15, 2017

Oh that's neat too! But my main point is that Browserify tries to maintain strict compatibility with Node but in this instance I believe that compatibility would be compromised.

I think this be quite compelling as a Browserify transform, however. brfs does this a little but only mocks fs.readFileSync/fs.readFile whereas with memfs you could collect all sources up and make a virtual volume. What do you think?

@streamich
Copy link
Author

streamich commented Nov 15, 2017

Browserify tries to maintain strict compatibility with Node

All modules that do I/O (like fs, net, http etc.) will inevitably be different from Node's modules.

The thing is that Browserify currently has nothing as fs module substitute and memfs implements 100% of fs API. Browserify, for example, ships a path module implementation, but there is no fs module to actually use that.

I personally think brfs is more confusing. But there could be something like brfs that adds assets to memfs.

@goto-bus-stop
Copy link
Member

thanks for the PR! i also think browserify shouldn't provide a default fs implementation because there is no filesystem in the browser and pretending there is is always going to be surprising for some use cases. see also webpack/node-libs-browser#72 (comment)

I don't think there's a one-size-fits-all browser fs polyfill. a readFile call in the browser may have to do a request to a server like in BrowserFS, if the file is a template or image resource. If the file is a user configuration file, using a client-side store like browserify-fs makes more sense. that makes it difficult to pick a single polyfill.

something like path may not be super useful in the browser, but the functionality of the module itself is consistent with what you would get in node.

you can still use memfs in projects where it makes sense by putting this in your package.json:

{
  "browser": { "fs": "memfs" }
}

and yeah @casr's idea about a transform that uses memfs to create a virtual filesystem sounds rad too!

@streamich
Copy link
Author

@goto-bus-stop The point of Browserify is to provide Node's API in the browser. All Node modules that Browserify shims don't make much sense in the browser but are required to make NPM modules work. So, I don't really see your point.

@goto-bus-stop
Copy link
Member

there are expectations around how core modules work. path manipulation and url parsing work the same way in node and the browser, so providing those by default is harmless.

the http module in node can do two things: make http requests and respond to http requests. the first is also possible in the browser, using xmlhttprequest, so browserify provides an implementation for http.request that behaves the same way as in node. responding to http requests isn't really possible in the browser though, so browserify doesn't provide http.createServer. you can sort of fake a server in the browser using serviceWorkers, and there are modules that do this--but it should not be the default, since the actual effect of the methods is very different from node. if i can do http.createServer(), i expect to be able to listen to a port of my choosing, and receive requests from other machines etc, but none of those expectations hold up for a serviceWorker based http.createServer() implementation.

i think memfs is more widely useful but some of the same issues apply when thinking about making it default behaviour. when i write a file, sometimes i expect it to be always available, sometimes i expect it to disappear after a reboot, sometimes i expect it to be available to other programs, sometimes i expect it to be edited by the user, etc, and all these possible expectations are impossible to satisfy in the browser with just the Node fs API.

i guess my point is that, while memfs provides the same API, it provides a different functionality--functionality that is a good alternative in some cases but not others. other browserify modules provide the same API and the same functionality as the core equivalents, and if some methods cannot provide the same functionality, they aren't provided at all.

@streamich
Copy link
Author

@goto-bus-stop

path manipulation and url parsing work the same way in node and the browser, so providing those by default is harmless.

path and url modules don't do any I/O they are pure compute modules written in JavaScript, you can stick any compute module into browser and it will just work, because all it does is compute.

The problem is with I/O, JavaScript does not do I/O. That's why Node.js was created in the first place.

No Node.js I/O module can be ported to browser and behave the same. Your mentioned example is incorrect, in the sense that http module does not provide the same behaviour in Browserify as in Node.js; it is still different. The way Node does GET request and the way browser does it is completely different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants