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(volume): support the recursive option for fs.watch() #902

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

alienzhou
Copy link
Contributor

@alienzhou alienzhou commented Apr 5, 2023

The fs.watch API has recursive option but it did not implement it. For example, the watching listener below will not be triggered:

const vol = new Volume();
vol.mkdirSync('/test');

vol.watch('/', { recursive: true }, (eventType, filename) => {
  // won't be triggered
  console.log('fs.watch', eventType, filename);
});

setTimeout(() => {
  vol.writeFileSync('/test/lol.txt', '2');
}, 1000);

Besides when a child file's content (not recursive) is changed, the listener will not be triggered too:

const vol = new Volume();
vol.mkdirSync('/test');
vol.writeFileSync('/test/lol.txt', 'foo')

vol.watch('/test', { recursive: false }, (eventType, filename) => {
  // won't be triggered
  console.log('fs.watch', eventType, filename);
});

setTimeout(() => {
  vol.writeFileSync('/test/lol.txt', 'bar');
}, 1000);

Resolves #883

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 5, 2023

Does this resolve #883 by chance?

@alienzhou
Copy link
Contributor Author

Does this resolve #883 by chance?

Yes. I added a new test case for #883 just now.

@G-Rath G-Rath merged commit c829803 into streamich:master Apr 6, 2023
12 checks passed
streamich pushed a commit that referenced this pull request Apr 6, 2023
# [3.5.0](v3.4.13...v3.5.0) (2023-04-06)

### Features

* support the `recursive` option for `fs.watch()` ([#902](#902)) ([c829803](c829803))
@streamich
Copy link
Owner

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 2, 2023

fyi this is causing the tests that were added as part of streamich/unionfs#557 in unionfs to fail - I'm not sure if that is because of a bug in this code, in unionfs, or in the test suite

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 2, 2023

I think this implementation is buggy

Running the test using the native file system results in the file watcher being called only once and with `foo.js` (instead of twice and with `/tmp/foo.js`)
import { Volume } from '../volume';

function sleep(millisec: number): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    setTimeout(resolve, millisec);
  });
}

describe('volume', () => {
  it('should create a watcher', async () => {
    const ufs = require('fs');
    ufs.writeFileSync('/tmp/foo.js', 'hello test');

    const mockCallback = jest.fn();
    const writtenContent = 'hello world';
    const watcher = ufs.watch('/tmp/foo.js', mockCallback as any);

    try {
      ufs.writeFileSync('/tmp/foo.js', writtenContent);

      await sleep(3000);

      expect(mockCallback).toBeCalledTimes(2);
      expect(mockCallback).toBeCalledWith('change', '/tmp/foo.js');
    } finally {
      watcher.close();
    }
  });
});
Running the test with `memfs` _before_ this change results in the test passing
import { Volume } from '../volume';

function sleep(millisec: number): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    setTimeout(resolve, millisec);
  });
}

describe('volume', () => {
  it('should create a watcher', async () => {
    const ufs = Volume.fromJSON({ '/tmp/foo.js': 'hello test' });
//    const ufs = require('fs');
    ufs.writeFileSync('/tmp/foo.js', 'hello test');

    const mockCallback = jest.fn();
    const writtenContent = 'hello world';
    const watcher = ufs.watch('/tmp/foo.js', mockCallback as any);

    try {
      ufs.writeFileSync('/tmp/foo.js', writtenContent);

      await sleep(3000);

      expect(mockCallback).toBeCalledTimes(2);
      expect(mockCallback).toBeCalledWith('change', '/tmp/foo.js');
    } finally {
      watcher.close();
    }
  });
});
Running the test with `memfs` _after_ this change results in the test failing as being called with an empty string twice
import { Volume } from '../volume';

function sleep(millisec: number): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    setTimeout(resolve, millisec);
  });
}

describe('volume', () => {
  it('should create a watcher', async () => {
    const ufs = Volume.fromJSON({ '/tmp/foo.js': 'hello test' });
//    const ufs = require('fs');
    ufs.writeFileSync('/tmp/foo.js', 'hello test');

    const mockCallback = jest.fn();
    const writtenContent = 'hello world';
    const watcher = ufs.watch('/tmp/foo.js', mockCallback as any);

    try {
      ufs.writeFileSync('/tmp/foo.js', writtenContent);

      await sleep(3000);

      expect(mockCallback).toBeCalledTimes(2);
      expect(mockCallback).toBeCalledWith('change', '/tmp/foo.js');
    } finally {
      watcher.close();
    }
  });
});

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

Successfully merging this pull request may close these issues.

.rename/.renameSync could not trigger FSWatcher when changes are recursive
3 participants