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

use memfs instead of memory-fs #366

Closed
wants to merge 2 commits into from
Closed

Conversation

heygrady
Copy link

@heygrady heygrady commented Feb 9, 2019

Uses memfs instead of memory-fs for the in-memory outputFileSystem. memfs appears to be actively maintained, while memory-fs hasn't had any meaningful updates in a few years.

This allows people to use out-of-the-box webpack-dev-server for novel use-cases where having a fully API-compliant filesystem would be beneficial. Related: webpack/memory-fs#43

I ran into issues adding my own server-side-rendering middleware into webpack-dev-server that needed to require files from the outputFileSystem. Doing this with memory-fs was difficult, time consuming and seemed not to work very reliably. My alternative was to write to disk, which means 30 second re-builds.

Interestingly, memfs is part of a suite of tools that enables useful stuff like patching require to import modules from an in-memory file system.

This was a simple drop-in replacement.

  • swap memory-fs with memfs
  • move the join and normalize methods from memory-fs to "a better place"
  • All tests pass

@jsf-clabot
Copy link

jsf-clabot commented Feb 9, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 9, 2019

Codecov Report

Merging #366 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   96.85%   96.87%   +0.02%     
==========================================
  Files           7        7              
  Lines         254      256       +2     
==========================================
+ Hits          246      248       +2     
  Misses          8        8
Impacted Files Coverage Δ
lib/fs.js 95.45% <100%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e54af2...89524ae. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 9, 2019

Codecov Report

Merging #366 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   96.85%   96.89%   +0.04%     
==========================================
  Files           7        7              
  Lines         254      258       +4     
==========================================
+ Hits          246      250       +4     
  Misses          8        8
Impacted Files Coverage Δ
lib/fs.js 95.65% <100%> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e54af2...fbc4932. Read the comment docs.

lib/fs.js Outdated
const { fs: memfs } = require('memfs');
// borrow join and normalize from memory-fs
memfs.join = require('memory-fs/lib/join');
memfs.normalize = require('memory-fs/lib/normalize');
Copy link
Member

Choose a reason for hiding this comment

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

It is not good solution, we don't need two memory-fs package

Copy link
Author

Choose a reason for hiding this comment

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

Those functions appear to be unrelated to the file system itself and could live somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but it is bad have two memory fs package, we should move these function to separate repo or move to memfs

@alexander-akait
Copy link
Member

Also can you provide example where

I ran into issues adding my own server-side-rendering middleware into webpack-dev-server that needed to require files from the outputFileSystem. Doing this with memory-fs was difficult, time consuming and seemed not to work very reliably. My alternative was to write to disk, which means 30 second re-builds.

@heygrady
Copy link
Author

heygrady commented Feb 9, 2019

Original comment moved here: https://gist.github.com/heygrady/ae6ec0d725c6c68974802ba5d8328617


difficult, time consuming and seemed not to work very reliably.

This is what I meant:

  1. In order to import a file from memory-fs you need to use require-from-string
  2. When you use require-from-string you can't use code-splitting
  3. When you use require-from-string you can't use node-externals (sometimes)
  4. When you read from a buffer, everything becomes async
  5. Using fs-monkey with memfs and unionfs would side-step all of those problems.
// importFromMemoryFs.js
const { promisify } = require('util')
const requireFromString = require('require-from-string')

let cache = {}

const importFromMemoryFs = async (stats, pathname) => {
  const { compilation, hash } = stats
  const { outputFileSystem } = compilation.compiler
  if (cache[hash] && cache[hash][pathname]) {
    return cache[hash][pathname]
  } else if (!cache[hash]) {
    cache = { [hash]: {} }
  }
  const readFileAsync = promisify(outputFileSystem.readFile.bind(outputFileSystem))
  try {
    const buffer = await readFileAsync(pathname, 'utf8')
    const source = buffer.toString()
    const module = requireFromString(source, pathname)
    cache[hash][pathname] = module
    return module
  } catch (error) {
    console.error(error)
  }
}

module.exports = importFromMemoryFs

In my project I'm using webpack-dev-server with server-side-rendering. I was able to get it all working roughly as described here.

// ssr-dev-middleware.js
const path = require('path')
const { wrap } = require('async-middleware')

const importFromMemoryFs = './importFromMemoryFs'

const pathToMain = path.join(__dirname, '..', 'dist', 'server', 'main.js')

module.exports = wrap(async (req, res, next) => {
  const stats = res.locals.webpackStats.stats[1]
  const middleware = await importFromMemoryFs(stats, pathToMain)
  middleware(req, res, () => undefined)
})

@heygrady
Copy link
Author

heygrady commented Feb 9, 2019

streamich/memfs#325 (comment)

I created a fork of memory-fs and tried to replace it with Volume. It's mostly working but some tests are failing. Most of the test failures are related to streams.

There are also some failures related to windows-style paths. I think those might be due to memory-fs using a normalize function on all paths and I didn't wrap the memfs methods to do that.

I'm exploring easy ways to make memory-fs API-compliant to work with webpack-dev-server.

https://github.com/heygrady/memory-fs/tree/memfs


Update

webpack/memory-fs#66

I was able to get every test passing except for the tests with win32 paths starting with C:\. It appears that memfs doesn't handle those.

Some differences:

  • memfs streams behave like fs does. This causes some tests to fail where memory-fs streams were non-standard.
  • memfs has a concept of "relative" paths that uses process.cwd (similar to fs). This causes some tests to fail where memory-fs normally throws on relative paths.
  • memfs does not handle win32 paths starting with C:\ (when you are on a posix machine).

Switching to memfs would probably be blocked by the difference in win32 support. I might've overlooked something obvious but the memory-fs tests with paths starting with C:\ totally failed with memfs.

@hinell
Copy link
Contributor

hinell commented Feb 18, 2019

This is nice PR but I got far better solution I believe. #370

@alexander-akait
Copy link
Member

@hinell yep, #370 better

If somebody have problem with memory-fs please open issue

@laynef
Copy link

laynef commented Jun 28, 2019

@hinell yep, #370 better

If somebody have problem with memory-fs please open issue

I am currently having issues with maxing out memory, I've written my files to disk and that is not helping

@hinell
Copy link
Contributor

hinell commented Jun 28, 2019

@laynef It's better to open a separate issue on that. It would be also nice to elaborate it with details. So far everything it worked smoothly for me.

@alexander-akait
Copy link
Member

You can configure fs using options, maybe we revisit default memory fs in future, no need right now

@alexander-akait
Copy link
Member

Anyway thank you for the PR

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

Successfully merging this pull request may close these issues.

None yet

5 participants