Fix watch mode to listen to changes below the "longest common directory prefix" of relevant files, rather than only files below process.cwd()
, while keeping event filtering intact
#9267
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This fixes a regression in
--watch
mode which was introduced in #9009. The previous behavior before #9009 was to watch all relevant files with Chokidar, but when #9009 switched to@parcel/watcher
, the behavior changed to listen for all events belowprocess.cwd()
and then filter them for relevant files. This is a good approach, but it's a regression for some configurations that validly refer to files outside of the current working directory (e.g.documents: "../some-other/*.graphql"
). Configurations with such paths continue to build correctly, but the bug was that any files referenced by those paths would not be included in watch mode.The fix is to listen to events on all file changes below the "longest common directory prefix" of all relevant files, instead of only listening to events on all file changes below
process.cwd()
. An alternate fix would be to add something likeconfig.watchDirectoryRoot
, but that would still be a regression (or at least, it would be a breaking change) from pre-#9009, since there should be no reason previously valid configurations should need to add an extraneous config value to work as they were before.Note this does not alter the logic related to filtering file change events, once they are received, so e.g.
config.watchPattern
will continue to work as expected. This only changes the root directory given toparcelWatcher.subscribe()
. Filtering continues to happen after receiving events in theParcelWatcher.SubscribeCallback
function.Related #9266
Maybe related #9233
Type of change
How Has This Been Tested?
Test from this PR
I've tested this in my own repository. I also tested it in this repository by adding a
dev-test-outer-dir
with a*.graphql
file in it, adding it as adocument
to one of the stanzas indev-test/codegen.ts
, and adding ayarn watch:examples
script which is the watch mode equivalent ofyarn generate:examples
. I didn't see any way to properly test the watching logic, so this was the best I could do without a much larger PR. At least this way we can manually check for regressions in the future by seeing ifyarn watch:examples
oryarn generate:examples
incorrectly output a diff fromHEAD
. Please let me know if you have any better ideas.Here's how you can test it, after pulling this branch:
Running
git status
should show no changes. Then, you can test watch mode:You should see output which includes:
This is because the repo root is the "longest common directory prefix" of
./dev-test
and./dev-test-outer-dir
.While this is running, edit (save) the file
dev-test-outer-dir/githunt/current-user.query.graphql
, which is "outside" of./dev-test/codegen.ts
. This should trigger a rebuild:Running
git status
should show no changes (unless of course you actually made a meaningful change to the.graphql
file, rather than just touching/saving it).Click here for a more involved test where you actually `cd` into `dev-test` and update `codegen.ts` to use relative paths
The problem with the test described above
The problem with the approach above is that
process.cwd()
is always going to be the repository root, so you can't really tell if it's working correctly or just falling back toprocess.cwd()
, since the listener root will be the repo root in all cases. (Note: The original motivating issue for this bug was in a yarn workspaces repo, where running a command withyarn workspace
changes the current working directory to the workspace, i.e. the sub-package containing our graphql configs.)Try a more detailed test
I have a commit on another branch that updates all
./dev-test
paths referenced inside of./dev-test
to instead reference./
(and also contains a change to callprettier
from../node_modules/.bin/prettier
). To test the changes here, you can do the following:First, checkout the branch with these changes from my fork and peek at the diff:
Then,
cd
intodev-test
and run the generator while the current working directory isdev-test
instead of the repo root:cd dev-test node ../packages/graphql-codegen-cli/dist/cjs/bin.js --require dotenv/config --config ./codegen.ts dotenv_config_path=.env --watch
You should see watch mode listening for changes in the repo root:
(Give it a second for the prettier changes to apply before running
git status
again.)This is because of the referenced
'../dev-test-outer-dir/githunt/**/*.graphql'
document. You can remove that:And then run the watch mode again:
cd dev-test node ../packages/graphql-codegen-cli/dist/cjs/bin.js --require dotenv/config --config ./codegen.ts dotenv_config_path=.env --watch
This time, you should see it watching for changes in
dev-test
, because there are nodocuments
referenced outside of that directory (since you just removed the only one of them with thatgit apply
command):This testing shows the PR working as expected.
(end of more detailed test)
Test Environment:
@graphql-codegen/cli
: This PRChecklist:
yarn watch:examples
]Further comments
PR Structure
This PR is separated into two commits. The first commit (4e171d2 at time of writing) contains the actual fix, and all the code is in
./packages/graphql-codegen-cli/src/utils/watcher.ts
. The second commit (0a64f6b at time of writing) is a bit messier, and it contains the addition of theyarn watch:examples
script anddev-test-outer-dir
directory.Safety Mechanism
If the path about to be returned by
findHighestCommonDirectory
is not accessible (according tofs.access
, which is the recommended best practice for checking accessibility), then it will returnprocess.cwd()
instead, effectively falling back to current behavior.Note that falling back to current behavior would still be a regression from pre-#9009. Perhaps it would be worthwhile to also add a
config.watchRootPath
option to explicitly override this, and to prefer that when it's set, while otherwise usingfindHighestCommonDirectory
(with theprocess.cwd()
fallback if it's about to return an inaccessible path). But I wanted to keep this PR small, and I think this is the safest code that fixes the regression introduced in #9009.longestCommonPrefix
logicFor the
longestCommonPrefix
function, obviously this is a common leetcode problem, so I adapted the solution from this medium post, which seemed to work fine in my testing.findHighestCommonDirectory
andmicromatch.scan
logicThe logic in
findHighestCommonDirectory
maps eachfile
infiles
(which can be both relative and absolute paths or micromatch patterns) tomicromatch.scan(file).base
. This is only partially documented, but in my testing, it seems to be what we want: the.base
value is the longest directory before any wildcard segment. I included some examples in a comment above the code that uses it.