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: support unwatchFile #557

Merged
merged 1 commit into from
Jun 18, 2022
Merged

Conversation

aleung
Copy link
Contributor

@aleung aleung commented Feb 8, 2021

  • implement unwatchFile
  • add unit test case for it
  • format the code with prettier

@tstordyallison
Copy link

tstordyallison commented Feb 22, 2021

+1 for this if anyone is around to review...

I stumbled across the error and it led me here!

@G-Rath G-Rath changed the title Implement unwatchFile feat: support unwatchFile Jun 18, 2022
@G-Rath G-Rath merged commit 721f49d into streamich:master Jun 18, 2022
@G-Rath G-Rath mentioned this pull request Jun 20, 2022
@G-Rath
Copy link
Collaborator

G-Rath commented Jun 20, 2022

@aleung it looks like your test might be a bit unstable - would you be willing to look into that for us?

@aleung
Copy link
Contributor Author

aleung commented Jun 20, 2022

@G-Rath The test failed in "union › Union › Streams › can create Writable Streams" but I didn't change this one. The test added in my PR is "union › Union › sync methods › watch() › can watchFile and unwatchFile". Anyway, I'll try to run test locally to verify it.

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 20, 2022

@aleung yeah that failure is a different one. The failure from this PR seems to only happen when you run the tests multiple times (so won't show up in CI) due to files being created so it looks like it behaves differently on subsequent runs.

Thanks for looking into this! I'm a bit slammed at the moment so any time you can spare is appreciated 😊

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 24, 2022

@aleung it looks like the issue was just timing - adding another call to sleep between setting up the watcher and the initial writing to file looks to have resolved the issue.

@aleung
Copy link
Contributor Author

aleung commented Jun 29, 2022

@G-Rath Great. Do you need any more action from me?

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 2, 2023

@aleung at the time no, but now it looks like after a general lockfile update (#758) a new issue has start occuring:

● union › Union › sync methods › watch() › should create a watcher

    expect(jest.fn()).toBeCalledWith(...expected)

    Expected: "change", "/tmp/foo.js"
    Received
           1: "change", ""
           2: "change", ""

    Number of calls: 2

       95 |
       96 |           ufs.use(fs).use(Volume.fromJSON({ 'foo.js': '' }, '/tmp') as any);
    >  97 |
          | ^
       98 |           expect(ufs.existsSync(__filename)).toBe(true);
       99 |           expect(fs.existsSync(__filename)).toBe(true);
      100 |           expect(ufs.existsSync('/tmp/foo.js')).toBe(true);

      at Object.<anonymous> (src/__tests__/union.test.ts:97:42)

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 2, 2023

It looks like the issue is because of streamich/memfs#902

github-actions bot pushed a commit that referenced this pull request Jun 2, 2023
# [4.5.0](v4.4.0...v4.5.0) (2023-06-02)

### Features

* support `unwatchFile` ([#557](#557)) ([721f49d](721f49d))
@G-Rath
Copy link
Collaborator

G-Rath commented Jun 2, 2023

This has now been released in 4.5.0 🎉

@aleung aleung deleted the feat_unwatchFile branch June 8, 2023 14:41
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

3 participants