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

chore: rewrite file watcher #32

Closed
wants to merge 1 commit into from

Conversation

yurks
Copy link
Contributor

@yurks yurks commented Jun 7, 2024

Currently, file watcher asynchronously retrieves documents and schemas on each project file changes, which is slow, and caused unreasonably high resources usage.

Changes made

  • resolve all configured documents and schemas only once on server startup and simply check file path matching in watcher.
  • fix cases when schema path defined with dot like ./src/schema.graphql by resolve absolute schema paths.
  • fix cases when documents defined with glob pattern like ./src/**/*.graphql, and modifying src/path/api.graphql.generated.ts file caused false positive check and trigger watcher for that file.

Also, this PR should resolve OOM issues reported in #27

Side changes

  • reduce amount of log records, as currently it contains a lot of records which is not related to plugin: modifying any file in project produces ~5 log entries. Below are screenshots of the same number of interactions with project files demonstrating this.

Before

image

After

image

@danielwaltz
Copy link
Owner

Thanks for your effort on these PRs! Hope to carve out the time in the next week or so to test them out and get them merged. 🎉

@danielwaltz danielwaltz self-requested a review June 7, 2024 18:45
Copy link
Owner

@danielwaltz danielwaltz left a comment

Choose a reason for hiding this comment

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

I love the idea behind this, and I completely agree that doing all these file checks on every change is quite excessive (and I'm a little embarrassed I didn't see that myself before).

Unfortunately a result of this change is that adding new document files no longer triggers codegen, which would mean this is a breaking change. If there is a way we can update the cache when new document files are added (and deleted perhaps), that would be fantastic!

I have some (pretty hacky feeling) tests written to test this basic functionality locally. If you run npm run test locally you should see the generates on file add test fail and this can be used to help ensure the feature is still working!

danielwaltz added a commit that referenced this pull request Dec 1, 2024
github-actions bot pushed a commit that referenced this pull request Dec 1, 2024
### [3.4.1](v3.4.0...v3.4.1) (2024-12-01)

### Performance Improvements

* load and cache matches on server start ([69c1d97](69c1d97)), closes [#32](#32) [#27](#27)
* skip match cache refresh if file is generated ([c0737a7](c0737a7))

### Tests

* scope vite instances to spec directories ([90d771f](90d771f))

### Miscellaneous Chores

* **deps:** update deps ([43ed87b](43ed87b))
* **deps:** use vite 6 ([64c89c9](64c89c9))
* update dev deps ([2de28d2](2de28d2))
@danielwaltz
Copy link
Owner

Thanks again for the work on this! I have implemented this strategy in a slightly different way that still supports the use cased of adding files. Please let me know if you run into any issues in the latest version!

@danielwaltz danielwaltz closed this Dec 1, 2024
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.

2 participants