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.

Sign up for GitHub

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

Jump to bottom

--watch Only watches files below process.cwd(), even though some paths may be outside of it #9266

Closed
milesrichardson opened this issue Apr 3, 2023 · 5 comments · Fixed by #9269
Closed

--watch Only watches files below process.cwd(), even though some paths may be outside of it #9266

milesrichardson opened this issue Apr 3, 2023 · 5 comments · Fixed by #9269
Labels
stage/5-alpha-release-testing The pull request is merged, an alpha release is available, to be tested

Comments

@milesrichardson
Copy link
Contributor

milesrichardson commented Apr 3, 2023 •
edited

Which packages are impacted by your issue?

@graphql-codegen/cli

Describe the bug

Watch mode only watches files below process.cwd(), even though the codegen config may validly reference files outside of this directory.

Bug

This bug was introduced in #9009. Previously, when using Chokidar, watch mode listened for changes in every relevant file. However, after switching to @parcel/watcher, watch mode only listens for changes below process.cwd(), and then filters each change to see whether it's a relevant file. This is a fine approach, except that it's possible and valid for a config to reference paths outside of the current directory. When calling graphql-codegen generate, the bug does not exist, and files outside of process.cwd() are included in the generation. Therefore, this is a regression in watch mode only.

Reproduction

I have a minimal reproduction, but unfortunately watch mode doesn't work in Stackblitz, so you need to pull the repo and run it locally to see it reproduce:

Fix

The fix, if we want to match the previous behavior, is to find the "longest common prefix" directory of all relevant files, and watch that directory instead of process.cwd(). An alternative solution would be to allow the user to specify a config.watchDir, but this would still be a regression from previous behavior.

I have a PR which implements the "longest common prefix." I will open it shortly, after I submit this issue.

EDIT: A pull request is pending review: #9267

Your Example Website or App

https://www.github.com/milesrichardson/repro-npm-graphql-codegen-watch-outside-cwd

Steps to Reproduce the Bug or Issue

See readme of reproduction repo.

Expected behavior

As a user, I expect that if I can reference a path outside of the current directory (e.g. documents: "../somewhere-else/*.graphql"), and if this correctly generates output when calling graphql-codegen, that calling graphql-codegen --watch should also include that path in watch mode. However, the bug is that only files below the current working directory are included in watch mode, even though they are correctly included in build mode.

Screenshots or Videos

No response

Platform

  • OS: all
  • NodeJS: all
  • graphql version: irrelevant
  • @graphql-codegen/cli version(s): 3.2.2

Codegen Config File

No response

Additional context

No response
milesrichardson added a commit to milesforks/graphql-code-generator that referenced this issue Apr 3, 2023
@milesrichardson
fix: Listen for file changes in watch mode in longest common directory
450cf00 
Fixes dotansimha#9266

Prior to dotansimha#9009, which moved from Chokidar to `@parcel/watcher`, the behavior
was to watch for all relevant files. However, since switching to
`@parcel/watcher`, the new behavior has been to watch all files below
`process.cwd()`, and then filter the change events for only the relevant files.
This approach works fine, except that it's possible for a valid config to
reference paths outside of the current working directory (e.g. `documents:
"../some-other/*.graphql`), and these paths are included in build mode, but
were not being included in watch mode because they're outside of
`process.cwd()`.

This commit adds logic, after parsing all relevant file paths, to find the
"longest common directory prefix" of all those file paths, i.e. the "highest"
(closest to `/`) directory that contains all the relevant file paths. Then,
when subscribing to the Parcel watcher, this directory is used instead of
`process.cwd()`. For example, the longest common directory of the paths
`/foo/bar/*.graphql` and `/foo/fizz/*.graphql` would be `/foo`.

Note that the filtering behavior is left unchanged, and this only affects the
root path given to the Parcel watcher. When an event is received, the filtering
can still filter out irrelevant paths, including those filtered by
`config.watchPattern` if it's defined.
This was referenced Apr 3, 2023
Merged
Open
milesrichardson added a commit to milesforks/graphql-code-generator that referenced this issue Apr 3, 2023
@milesrichardson
fix: Listen for file changes in watch mode in longest common directory
a1790b2 
Fixes dotansimha#9266

Prior to dotansimha#9009, which moved from Chokidar to `@parcel/watcher`, the behavior
was to watch for all relevant files. However, since switching to
`@parcel/watcher`, the new behavior has been to watch all files below
`process.cwd()`, and then filter the change events for only the relevant files.
This approach works fine, except that it's possible for a valid config to
reference paths outside of the current working directory (e.g. `documents:
"../some-other/*.graphql`), and these paths are included in build mode, but
were not being included in watch mode because they're outside of
`process.cwd()`.

This commit adds logic, after parsing all relevant file paths, to find the
"longest common directory prefix" of all those file paths, i.e. the "highest"
(closest to `/`) directory that contains all the relevant file paths. Then,
when subscribing to the Parcel watcher, this directory is used instead of
`process.cwd()`. For example, the longest common directory of the paths
`/foo/bar/*.graphql` and `/foo/fizz/*.graphql` would be `/foo`.

Note that the filtering behavior is left unchanged, and this only affects the
root path given to the Parcel watcher. When an event is received, the filtering
can still filter out irrelevant paths, including those filtered by
`config.watchPattern` if it's defined.
milesrichardson added a commit to milesforks/graphql-code-generator that referenced this issue Apr 3, 2023
@milesrichardson
fix: Listen for file changes in watch mode in longest common directory
e5ede16 
Fixes dotansimha#9266

Prior to dotansimha#9009, which moved from Chokidar to `@parcel/watcher`, the behavior
was to watch for all relevant files. However, since switching to
`@parcel/watcher`, the new behavior has been to watch all files below
`process.cwd()`, and then filter the change events for only the relevant files.
This approach works fine, except that it's possible for a valid config to
reference paths outside of the current working directory (e.g. `documents:
"../some-other/*.graphql`), and these paths are included in build mode, but
were not being included in watch mode because they're outside of
`process.cwd()`.

This commit adds logic, after parsing all relevant file paths, to find the
"longest common directory prefix" of all those file paths, i.e. the "highest"
(closest to `/`) directory that contains all the relevant file paths. Then,
when subscribing to the Parcel watcher, this directory is used instead of
`process.cwd()`. For example, the longest common directory of the paths
`/foo/bar/*.graphql` and `/foo/fizz/*.graphql` would be `/foo`.

Note that the filtering behavior is left unchanged, and this only affects the
root path given to the Parcel watcher. When an event is received, the filtering
can still filter out irrelevant paths, including those filtered by
`config.watchPattern` if it's defined.
@saihaj saihaj added the stage/5-alpha-release-testing The pull request is merged, an alpha release is available, to be tested label Apr 4, 2023
@theguild-bot theguild-bot mentioned this issue Apr 4, 2023
Merged
@saihaj
Copy link
Collaborator

saihaj commented Apr 4, 2023

can you test @graphql-codegen/cli@3.3.1-rc-20230404030530-47bcfb095

@milesrichardson
Copy link
Contributor Author

sure thing, will test it in the morning

@milesrichardson
Copy link
Contributor Author

I tested this in our repository, and it works as expected. I think you can cut that release.

However, in our case, this exposed a different bug, which sounds like what I've seen complaints about in a few other issues. Basically, the event filtering is using .contains(), but I believe it should be using .isMatch(), because .contains() includes partial matches. In our case this meant that when a rebuild caused changes in the .next directory, those changes were also included because they were a "partial match", e.g.:

Running lifecycle hook "onWatchTriggered" scripts...
[Watcher] triggered due to a file create event: /src/js/cloud/catalog/.next/cache/webpack/server-development/7.pack

I patched our local installation to switch to .isMatch and it seems to work as expected. I'll open an issue/PR for that bug/fix as well, but I'm slightly less confident in it because micromatch rules are notoriously finnicky and I don't know how everyone else is using them.

@milesrichardson milesrichardson mentioned this issue Apr 4, 2023
Open
@milesrichardson
Copy link
Contributor Author

Opened #9270 to track the new issue

@milesrichardson milesrichardson mentioned this issue Apr 8, 2023
Merged
9 tasks
@milesrichardson
Copy link
Contributor Author

Added some tests for this behavior (and fixed a few minor bugs) as part of #9275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stage/5-alpha-release-testing The pull request is merged, an alpha release is available, to be tested
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Upcoming Release Changes dotansimha/graphql-code-generator
2 participants
@milesrichardson @saihaj