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

Fix issue with onDidChange not always being called #143

Merged
merged 4 commits into from Apr 21, 2021
Merged

Fix issue with onDidChange not always being called #143

merged 4 commits into from Apr 21, 2021

Conversation

nhevia
Copy link
Contributor

@nhevia nhevia commented Feb 8, 2021

Fixes #108 and fixes sindresorhus/electron-store#93

Adds a configuration option called watchFile.

Works exactly like watch but uses fs.watchFile instead of fs.watch, allowing the subscription to read changes in the file more than once.

useEffect(()=>{
    store.onDidChange('myKey', (newV, oldV) => {
      console.log(`Changed from ${oldV} to ${newV}`)
    })
},[])

image

Clearly slower (sometimes, benchmarking is unreliable 100/2000ms) than watch but the only way to truly watch a file in React (and possible other scenarios, but not tested).

@alifarooq0
Copy link

Great work @nhevia! Really need this to fix sindresorhus/electron-store#93. @sindresorhus can we please get this in ASAP?

@sindresorhus
Copy link
Owner

So fs.watch only fails on Ubuntu, right? Can't we just use fs.watchFile on Linux? No option needed.

@sindresorhus
Copy link
Owner

Also please open an issue on Node.js. I want to ensure the issue will be eventually fixed in Node.js.

@alifarooq0
Copy link

So fs.watch only fails on Ubuntu, right? Can't we just use fs.watchFile on Linux? No option needed.

I'm seeing #108 on macOS.

@sindresorhus
Copy link
Owner

I'm seeing #108 on macOS.

Ok, so let's use fs.watch on macOS too then.

@alifarooq0
Copy link

you mean fs.watchFile? Sure, works for me 👍 . @nhevia are you able to make the necessary changes? It will be a lot nicer to have this work without having to specify an extra option :).

@sindresorhus
Copy link
Owner

Yes

@nhevia
Copy link
Contributor Author

nhevia commented Feb 13, 2021

Changes added.

Tests were failing, added to review.

Copy link

@alifarooq0 alifarooq0 left a comment

Choose a reason for hiding this comment

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

Wicked 👍

@alifarooq0
Copy link

Thanks for the quick turn around on this @nhevia!

test/index.ts Outdated Show resolved Hide resolved
test/index.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

The PR title is outdated.

@nhevia nhevia changed the title Add watchFile option Use watchFile for all OS except win32 Feb 20, 2021
@alifarooq0
Copy link

Will you have time to address the feedback @nhevia?

@sindresorhus
Copy link
Owner

@nhevia Bump

@nhevia
Copy link
Contributor Author

nhevia commented Apr 19, 2021

Tests passing. Feel free to squash this mess 👀

@sindresorhus sindresorhus changed the title Use watchFile for all OS except win32 Fix issue with onDidChange not always being called Apr 21, 2021
@sindresorhus sindresorhus merged commit 3a53356 into sindresorhus:main Apr 21, 2021
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.

onDidChange is called once onDidChange only fires once (reactjs)
3 participants