Skip to content

Add generic file watcher api #1802

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

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

pokey
Copy link
Member

@pokey pokey commented Aug 17, 2023

Checklist

@pokey pokey marked this pull request as ready for review August 17, 2023 00:43
@pokey pokey requested a review from AndreasArvidsson August 17, 2023 00:43
@AndreasArvidsson
Copy link
Member

I'm not in love with the polling implementation. I would prefer if we added support for file watchers if you have a recent enough version.

@pokey
Copy link
Member Author

pokey commented Aug 17, 2023

I'm not in love with the polling implementation. I would prefer if we added support for file watchers if you have a recent enough version.

Yeah I think probably not too much effort to do version check and use watcher if available. For now, this is only used for dev hot reloading, though, (ie the code doesn't even run in production), and I won't have much time to work on it for next few days as I'm travelling. Mind if we merge this so you / @josharian can have hot reloading and then I'll do quick follow-up early next week with proper watcher?

Fwiw this is the exact same mechanism we use in production for snippet reloading, and it's quite low resource utilisation as it's just a stat

@josharian
Copy link
Collaborator

Mind if we merge this so you / @josharian can have hot reloading and then I'll do quick follow-up early next week with proper watcher?

yes please!

@josharian
Copy link
Collaborator

tbh, for dev only, i don't see that it's worth the effort to switch from polling, if polling is already built.

@pokey
Copy link
Member Author

pokey commented Aug 17, 2023

We will be using it for the tree-view next, so I'll switch before we ship that

@pokey
Copy link
Member Author

pokey commented Aug 17, 2023

You can also just rebase onto the hot reloading PR while developing for now. That's what I did for my other PR

@AndreasArvidsson
Copy link
Member

How can I say no to josh? Commence the merging!

@AndreasArvidsson AndreasArvidsson added this pull request to the merge queue Aug 18, 2023
Merged via the queue into main with commit c10b417 Aug 18, 2023
@AndreasArvidsson AndreasArvidsson deleted the pokey/add-generic-file-watcher-api branch August 18, 2023 07:34
github-merge-queue bot pushed a commit that referenced this pull request Aug 18, 2023
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.

3 participants