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: always return a promise from the stopper function #296

Merged
merged 3 commits into from
Nov 9, 2019
Merged

feat: always return a promise from the stopper function #296

merged 3 commits into from
Nov 9, 2019

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Nov 9, 2019

As mentioned in #295, the return value of the stopper function seems inconsistent. This makes it always return Promise<null> regardless of when it's called.

An open question is - does Native.stop resolve with a value? I tested locally and it always resolved to undefined, so I'm thinking just resolving to null is fine. But I don't know C, so it might resolve to something else in some cases?

(I've also updated the readme with the new event from #286).

@paulmillr
Copy link
Member

I don't think this would work correctly. Recalling a case when I was editing some code and decided to do the same change. fsevents broke after it.

But I may be wrong. Did you test it on something?

@paulmillr paulmillr merged commit 6722e56 into fsevents:master Nov 9, 2019
@SimenB SimenB deleted the promise-null-stop branch November 9, 2019 20:57
@SimenB
Copy link
Contributor Author

SimenB commented Nov 9, 2019

But I may be wrong. Did you test it on something?

I just tested it locally, not especially thoroughly. I didn't actually know about the stop function until looking into it today 😬 Unsure why it would break anything though?

@paulmillr
Copy link
Member

There is some asynchronous code that may be getting called at some point. It’s all messy and related to race conditions - for example, when fsevents instances got closed already. Not really easy to test!

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

2 participants