Trigger rebuilds in watch mode while respecting rules of precedence and negation #9275
Conversation
🦋 Changeset detectedLatest commit: 96f3cfe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // Is path negated by any negating watch pattern? | ||
| if ( | ||
| matchesAnyAffirmativePattern( | ||
| path, | ||
| invertNegatedPatterns([...globalPatternSet.watch.negated, ...localPatternSet.watch.negated]) | ||
| ) | ||
| ) { | ||
| // Short circut: negations in watch patterns take priority | ||
| return false; | ||
| } | ||
|
|
||
| // Does path match any affirmative watch pattern? | ||
| if ( | ||
| matchesAnyAffirmativePattern(path, [ | ||
| ...globalPatternSet.watch.affirmative, | ||
| ...localPatternSet.watch.affirmative, | ||
| ]) | ||
| ) { | ||
| // Immediately return true: Watch pattern takes priority, even if documents or schema would negate it | ||
| return true; | ||
| } |
There was a problem hiding this comment.
For watch patterns, we separate the patterns into negated and affirmative patterns, and we evaluate them each separately. That is, we use micromatch only to evaluate affirmative patterns (which may be inverted negations). This is necessary only for the watch rules because we use them to short circuit the algorithm. For documents and schemas, we can evaluate rules using the - see below for more on that.micromatch default export (or rather, our thin wrapper of it, pathMatches)
| // Does path match documents patterns (without being negated)? | ||
| if (pathMatches(path, [...globalPatternSet.documents.patterns, ...localPatternSet.documents.patterns])) { | ||
| return true; | ||
| } | ||
|
|
||
| // Does path match schemas patterns (without being negated)? | ||
| if (pathMatches(path, [...globalPatternSet.schemas.patterns, ...localPatternSet.schemas.patterns])) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
For documents and schemas, we rely on micromatch, and we pass it a list of patterns that can include both affirmative (non-negated) and negated patterns.
Because micromatch is a bit finnicky, we can't use micromatch.isMatch, which is why we need to make this pathMatches function.
There was a problem hiding this comment.
After writing test cases, it turns out this was not true - we cannot rely on micromatch - so I deleted the patchMatches shortcut, in favor of explicitly checking the affirmative matches, and then checking there are no negated matches. But I actually prefer it this way, because now the logic matches the flow chart exactly.
The reason for this is that micromatch cares about order of rules, and a negation will only override a match when it comes after the affirmative rule. However, we do not care about order (right? or maybe we do care about order within local groups? let me know) - negations should always override affirmative matches.
See fix in b2328fc (but note the commit message body is inaccurate - the reason micromatch doesn't work has to do with ordering, not exact vs. glob match).
This shows why it didn't work:
// Given this config
globalPattern.watch = '!**/exclude-doc-everywhere.graphql'
localPattern.watch = 'foo/global-beats-local/exclude-doc-everywhere.graphql'
// And this path:
const path = 'foo/global-beats-local/exclude-doc-everywhere.graphql'
// We would expect this NOT to match, because the global negation should win
// However, micromatch considers this a match:
true === require('micromatch')(
[path],
[
'!**/exclude-doc-everywhere.graphql',
'foo/global-beats-local/exclude-doc-everywhere.graphql'
// Note: the same is true even with:
// '**/exclude-doc-everywhere.graphql',
],
{ cwd: process.cwd() }
).length === 1
// But when changing the order, it does not consider it a match
false === require('micromatch')(
[path],
[
'foo/global-beats-local/exclude-doc-everywhere.graphql',
// Note: the same is true even with:
// '**/exclude-doc-everywhere.graphql',
'!**/exclude-doc-everywhere.graphql'
],
{ cwd: process.cwd() }
).length === 1The expected behavior is now covered by a unit test (the same one which exposed the bug in the first place 😉 ): globally negated paths should be excluded even when a local pattern matches them
There was a problem hiding this comment.
However, we do not care about order (right? or maybe we do care about order within local groups? let me know) - negations should always override affirmative matches.
I actually think we care about the order e.g. if we have the following order
foo/foo.graphql
!foo/foo/bar/**/*
foo/foo/bar/asdf/foo.graphql
This should still include foo/foo/bar/asdf/foo.graphql IMHO and !foo/foo/bar/**/* should not overwrite it. is this the current behavior?
| return ({ path: absolutePath }: { path: string }) => { | ||
| if (!isAbsolute(absolutePath)) { | ||
| throw new Error('shouldRebuild trigger should be called with absolute path'); | ||
| } | ||
|
|
||
| const path = relative(process.cwd(), absolutePath); | ||
| const shouldRebuild = localMatchers.some(matcher => matcher(path)); | ||
| return shouldRebuild; | ||
| }; |
There was a problem hiding this comment.
This is the callback that is called on each path change. It loops over the pre-constructed matchers, one for each "local" (per-output target) pattern set. This way we avoid too much overhead on every file change, as the functions have all already been created. That's also why we sort/group the patterns into affirmative/negated only once, during initialization when we call sortPatterns, and store them in a PatternSet struct, rather than re-sorting them on each path change.
| return { | ||
| watch: sortPatterns([ | ||
| ...(typeof config.watch === 'boolean' ? [] : normalizeInstanceOrArray<string>(config.watch ?? [])), | ||
| relative(process.cwd(), initialContext.filepath), | ||
| ]), | ||
| schemas: sortPatterns(makePatternsFromSchemas(normalizeInstanceOrArray<Types.Schema>(config.schema))), | ||
| documents: sortPatterns( | ||
| makePatternsFromDocuments(normalizeInstanceOrArray<Types.OperationDocument>(config.documents)) | ||
| ), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
We set the global "watch" patterns to any defined in globalConfig.watch (if it's a non-boolean), and also add the path of the config file. Note that a relative path is used for the config file because we expect all patterns to use relative paths.
Note that we are "sorting" these patterns (grouping them into patterns/affirmative/negated) only once, on initialization, so that we can avoid doing that during every path change when we check if a rebuild should be triggered.
| /** | ||
| * Parse a list of micromatch patterns from a list of documents, which should | ||
| * already have been normalized from their raw config values. | ||
| */ | ||
| const makePatternsFromDocuments = (documents: Types.OperationDocument[]): string[] => { | ||
| const patterns: string[] = []; | ||
|
|
||
| if (documents) { | ||
| for (const doc of documents) { | ||
| if (typeof doc === 'string') { | ||
| patterns.push(doc); | ||
| } else { | ||
| patterns.push(...Object.keys(doc)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return patterns; | ||
| }; | ||
|
|
||
| /** | ||
| * Parse a list of micromatch patterns from a list of schemas, which should | ||
| * already have been normalized from their raw config values. | ||
| */ | ||
| const makePatternsFromSchemas = (schemas: Types.Schema[]): string[] => { | ||
| const patterns: string[] = []; | ||
|
|
||
| for (const s of schemas) { | ||
| const schema = s as string; | ||
| if (isGlob(schema) || isValidPath(schema)) { | ||
| patterns.push(schema); | ||
| } | ||
| } | ||
|
|
||
| return patterns; | ||
| }; |
There was a problem hiding this comment.
This logic is exactly the same as it was previously. It's just been moved to this file.
| if (!shouldRebuild({ path })) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is the crux of the matter: we call the previously constructed shouldRebuild() function on change events.
|
|
||
| let watcherSubscription: Awaited<ReturnType<typeof subscribe>>; | ||
|
|
||
| const runWatcher = async () => { | ||
| const watchDirectory = await findHighestCommonDirectory(files); | ||
| const watchDirectory = await findHighestCommonDirectory(allPatterns); | ||
|
|
||
| const parcelWatcher = await import('@parcel/watcher'); |
There was a problem hiding this comment.
I noticed that parcelWatcher supports an ignore parameter that takes micromatch patterns.
Unfortunately we still need this PR because of all the precedence/negation rules, but I think we could use the ignore pattern as an optimization, e.g. by inverting all global negations and adding them to ignore, and also adding some obvious patterns, like .git. This way we could avoid calling the shouldRebuild() function too often (although the overhead of that is hopefully not too high, we could at least avoid calling it for absurd times like when the user runs git status and .git/index.lock changes).
EDIT (2 days later): I realized we were already using the ignore pattern for the special case of ignoring output directories (e.g. when using presets like near-operation-file), which we turn into glob patterns ending with the extension. As part of my recent commits improving the test infrastructure, I added some assertion helpers for making sure we are adding the expected paths and/or globs to the ParcelWatcher.Options.ignore parameter.
| './dev-test/githunt/**/*.graphql', | ||
| './dev-test-outer-dir/githunt/**/*.graphql', | ||
| '!**/nothing-should-use-this-query.graphql', |
There was a problem hiding this comment.
I added the dev-test-outer-dir/githunt/nothing-should-use-this-query.graphql because it would otherwise match the pattern './dev-test-outer-dir/githunt/**/*.graphql', (which is only referenced once in this codegen.ts, making it a convenient choice), but it's negated by '!**/nothing-should-use-this-query.graphql',.
This is one of the crude smoke tests I used to see if the code is working, i.e. verifying that touch dev-test-outer-dir/githunt/nothing-should-use-this-query.graphql does not trigger a rebuild.
|
Looks like the workflow testing exports integrity failed. I guess this is probably due to my adding EDIT: Oh, I think I need to add the file extension to the import. Fixed and (force) pushed, I think. |
milesrichardson
force-pushed
the
watch-negated-patterns
branch
2 times, most recently
from
7637187
to
d46b216
Compare
|
Changeset added and force pushed. |
milesrichardson
force-pushed
the
watch-negated-patterns
branch
3 times, most recently
from
a04896b
to
5ca9063
Compare
| test('triggers rebuilds as expected (auto-assertions)', async () => { | ||
| const mockWatcher = await setupMockWatcher({ | ||
| filepath: './foo/some-config.ts', | ||
| config: { | ||
| schema: './foo/something.ts', | ||
| generates: { | ||
| ['./foo/some-output.ts']: { | ||
| documents: ['./foo/bar/*.graphql'], | ||
| }, | ||
| ['./foo/some-other-output.ts']: { | ||
| documents: ['./foo/some-other-bar/*.graphql'], | ||
| }, | ||
| ['./foo/some-preset-bar/']: { | ||
| preset: 'near-operation-file', | ||
| presetConfig: { | ||
| extension: '.generated.tsx', | ||
| baseTypesPath: 'types.ts', | ||
| }, | ||
| documents: ['./foo/some-preset-bar/*.graphql'], | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await assertBuildTriggers(mockWatcher, { | ||
| shouldTriggerBuild: [ | ||
| './foo/some-config.ts', // config file | ||
| './foo/bar/fizzbuzz.graphql', | ||
| ], | ||
| pathsWouldBeIgnoredByParcelWatcher: [ | ||
| // note: expectations should be relative to cwd; assertion helper converts | ||
| // the values received by parcelWatcher to match before testing them (see typedoc) | ||
| './foo/some-output.ts', // output file | ||
| 'foo/some-output.ts', // output file | ||
| ], | ||
| globsWouldBeIgnoredByParcelWatcher: [ | ||
| // note: globs are tested for exact match with argument passed to subscribe options | ||
| 'some-preset-bar/**/*.generated.tsx', // output of preset | ||
| ], | ||
| shouldNotTriggerBuild: [ | ||
| './foo/bar/something.ts', // unrelated file | ||
| './foo/some-output.ts', // output file (note: should be ignored by parcel anyway) | ||
| '.git/index.lock', | ||
| ], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Ok, so I got.... a bit carried away building the test infrastructure. 👀 I made a few minor changes to the watcher itself, but mostly just for supporting the test infrastructure. At this point the bulk of the PR is all test code.
The watch server is now fully mocked and testable. It can also be cleanly cancelled without any unhandled promises. And I added a whole bunch of helpers for making assertions on whether or not a file change event triggers a rebuild for a given config. That means we can write code like I've highlighted with this comment, which makes for really clean testing.
I think I'm now mostly done with the test infrastructure, and now I just need to add a bunch of cases and actually test it. 😅 But I've found and fixed a few bugs so far, so it's a good start.
There was a problem hiding this comment.
Here are some examples of what the error message formatting looks like (click to expand for screenshots):
Expected file to trigger build, but it didn't
Expected file NOT to trigger build, but it did
Expected path to be passed to ParcelWatcher.Options.ignore, but it was not
See: ParcelWatcher ignore option
Expected glob to be passed to ParcelWatcher.Options.ignore, but it was not
See: ParcelWatcher ignore option
milesrichardson
force-pushed
the
watch-negated-patterns
branch
from
47ed0b0
to
53c05b3
Compare
There was a problem hiding this comment.
I've completed this PR, and it's ready for review (pending some edits I'm about to make to the PR description). I also rebased it on the latest changes.
I realize there is a lot of code here, but note that almost all of it is test code (both mocking/assertion infrastructure, and also tests themselves), and the actual bulk of the algorithm from the flow chart is implemented in patterns.ts.
I added a lot of test cases, and fixed some bugs they revealed, and now I'm really confident this feature works as we want it to.
If you only review two files, the most important are: patterns.ts and watcher.spec.ts.
| // ParcelWatcher expects relative ignore patterns to be relative from watchDirectory, | ||
| // but we expect filename from config to be relative from cwd, so we need to convert | ||
| const filenameRelativeFromWatchDirectory = relative(watchDirectory, resolve(process.cwd(), entry.filename)); | ||
|
|
||
| if (entry.config.preset) { | ||
| const extension = entry.config.presetConfig?.extension; | ||
| if (extension) { | ||
| ignored.push(join(entry.filename, '**', '*' + extension)); | ||
| ignored.push(join(filenameRelativeFromWatchDirectory, '**', '*' + extension)); | ||
| } | ||
| } else { | ||
| ignored.push(entry.filename); | ||
| ignored.push(filenameRelativeFromWatchDirectory); | ||
| } | ||
| } |
There was a problem hiding this comment.
This fixes a bug that would have been introduced in #9266 (although anyone it affected wouldn't even have been able to even get as far as noticing it was a bug prior to that fix).
Specifically: ParcelWatcher.Options["ignore"] expects that any relative paths or globs are relative from the watchDirectory, but we were passing them relative to process.cwd() (since that's how they're defined in the config). But after #9266, it's possible for watchDirectory to be something other than process.cwd(), so we need to convert these paths to be relative from watchDirectory.
I added (a lot of...) test code to make assertions on which paths and globs are passed to ParcelWatcher.Options.ignore and added regression tests for this.
Related (not to watch mode, but similar idea): #9272
| const fullPath = join(watchDirectory, path); | ||
| // In ESM require is not defined | ||
| try { | ||
| delete require.cache[fullPath]; | ||
| delete require.cache[path]; | ||
| } catch (err) {} | ||
|
|
||
| if (eventName === 'update' && config.configFilePath && fullPath === config.configFilePath) { | ||
| if (eventName === 'update' && config.configFilePath && path === config.configFilePath) { | ||
| log(`${logSymbols.info} Config file has changed, reloading...`); |
There was a problem hiding this comment.
There was no need for fullPath here, because path is already absolute, and so concatenating it with watchDirectory was resulting in invalid paths (combining two absolute paths together). We can just use path instead. See f6ab511
| const shutdown = ( | ||
| /** Optional callback to execute after shutdown has completed its async tasks */ | ||
| afterShutdown?: () => void | ||
| ) => { | ||
| isShutdown = true; | ||
| debugLog(`[Watcher] Shutting down`); | ||
| log(`Shutting down watch...`); | ||
| watcherSubscription.unsubscribe(); | ||
| lifecycleHooks(config.hooks).beforeDone(); | ||
|
|
||
| const pendingUnsubscribe = watcherSubscription.unsubscribe(); | ||
| const pendingBeforeDoneHook = lifecycleHooks(config.hooks).beforeDone(); | ||
|
|
||
| if (afterShutdown && typeof afterShutdown === 'function') { | ||
| Promise.allSettled([pendingUnsubscribe, pendingBeforeDoneHook]).then(afterShutdown); | ||
| } |
There was a problem hiding this comment.
This adds an optional callback to the shutdown handler for it to call after it's completed its async tasks. In normal usage, this callback is not supplied, which makes sense because process.once() event handlers are not supposed to wait for any asynchronous code. And in that case, the logic is unchanged: each async operation is queued immediately and there is no waiting for them to complete (the program will just exit).
But in tests, we want to be able to cancel the watcher, and gracefully shutdown. So in that case we give the shutdown handler a callback, and we wait for the two asynchronous events (.unsubscribe(), and .beforeDone) to complete before we call the callback, so that we ensure no async tasks are remaining on the queue when we use that callback to resolve the pendingShutdown promise outside of this loop.
| abortSignal.addEventListener('abort', () => shutdown(abortSignal.reason)); | ||
|
|
||
| process.once('SIGINT', () => shutdown()); | ||
| process.once('SIGTERM', () => shutdown()); | ||
| }; | ||
|
|
||
| // Use an AbortController for shutdown signals | ||
| // NOTE: This will be polyfilled on Node 14 (or any environment without it defined) | ||
| const abortController = new AbortController(); |
There was a problem hiding this comment.
In tests, we make the watcher cancellable by using an AbortSignal. When running from the CLI, we don't use this, but we do still initialize new AbortController().
Unfortunately, AbortController is not available in Node v14 (unless an experimental flag is supplied to Node), so this caused some GitHub Actions to fail. I fixed this by adding a polyfill for AbortController. It will only be exported from abort-controller-polyfill.ts when executing in environments where global.AbortController is not already defined (which only includes Node v14 as long as the --experimental-abortcontroller flag is not passed), since otherwise abort-controller-polyfill.ts will re-export global.AbortController.
| executeCodegen(initialContext) | ||
| .then(onNext, () => Promise.resolve()) | ||
| .then(runWatcher) | ||
| .then(() => runWatcher(abortController.signal)) | ||
| .catch(err => { | ||
| watcherSubscription.unsubscribe(); | ||
| reject(err); | ||
| }) | ||
| .then(() => pendingShutdown) | ||
| .finally(() => { | ||
| debugLog('Done watching.'); | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There are some subtleties to this shutdown order, because the two promises rely on each other and there is some lazy property assignment. The gist of it is that await stopWatching() will only resolve after the runningWatcher has resolved, and the intermediate step of runningWatcher will wait for pendingShutdown, which is ultimately the callback passed to the shutdown handler, to complete.
I tested this extensively with a bunch of log messages, and every time I ran tests, I passed --detectOpenHandles, so I'm very confident that this shutdown logic works correctly. Namely: it's a graceful shutdown, and there are no events that leak after stopWatching has resolved.
| * NOTE: This mock does _not_ implement the Parcel Watcher `shouldIgnore` check, | ||
| * because that's implemented by Parcel Watcher in C++ and there is no sense | ||
| * duplicating it in JS. So if you want to make assertions about ignored paths, | ||
| * you should limit it to assertions about which paths end up in `subscribeOpts.ignore`, | ||
| * and otherwise assume that Parcel Watcher will work as expected. For making | ||
| * these assertions, see the `assertBuildTriggers` helper. | ||
| */ |
There was a problem hiding this comment.
Just highlighting this point about the mock: we don't implement ParcelWatcher's shouldIgnore, so even if paths/globs are passed to the ignore pattern, the subscribe callback will still be executed in test mode. To make up for this, we have assertion helpers for making assertions about which paths/globs are passed to ignore, and we just assume ParcelWatcher will implement its behavior correctly.
milesrichardson
changed the title
|
@saihaj This PR is complete and ready for review (and merge IMO 😄). Sorry it's so long. Most of it is test code or helpers for mocking and assertions. I updated the description with some bullet points. The most important files are I also did a self-review on all my code, which also makes the PR look even longer and more intimidating, but feel free to ignore those comments. |
milesrichardson
force-pushed
the
watch-negated-patterns
branch
2 times, most recently
from
4328901
to
c45e68c
Compare
|
One thing, follow-up to earlier comment: #9275 (comment) I said that we don't care about order of micromatch patterns, which is why I stopped using micromatch for matching within each stanza, but I'm now wondering if that's true. The docs say:
I think For example: .yarn/*
!.yarn/patches
!.yarn/plugins
!.yarn/releasesThis will "match" (ignore, confusingly) // This test PASSES
test('local negations in schema set should override match in same schema set', async () => {
const mockWatcher = await setupMockWatcher({
filepath: './foo/some-config.ts',
config: {
schema: './foo/something.ts',
generates: {
['./foo/some-output.ts']: {
schema: ['./foo/bar/*.graphql', '!./foo/bar/never.graphql'],
},
},
},
});
expect(mockWatcher.watchDirectory).toBe(join(process.cwd(), 'foo'));
await assertBuildTriggers(mockWatcher, {
shouldTriggerBuild: ['./foo/bar/okay.graphql'],
shouldNotTriggerBuild: ['./foo/bar/never.graphql'],
});
});But what we don't have is the inverse, where we exclude a glob and then selectively include from it: // This test FAILS
test.only('glob negations in schema set should be overridden by local matches', async () => {
const mockWatcher = await setupMockWatcher({
filepath: './foo/some-config.ts',
config: {
schema: './foo/something.ts',
generates: {
['./foo/some-output.ts']: {
schema: [
'!./foo/bar/ignore-everything-but-apple/*.graphql',
'./foo/bar/ignore-everything-but-apple/apple.graphql',
],
},
},
},
});
expect(mockWatcher.watchDirectory).toBe(join(process.cwd(), 'foo'));
await assertBuildTriggers(mockWatcher, {
shouldTriggerBuild: ['./foo/bar/ignore-everything-but-apple/apple.graphql'],
shouldNotTriggerBuild: [
'./foo/bar/ignore-everything-but-apple/orange.graphql',
'./foo/bar/ignore-everything-but-apple/banana.graphql',
],
});
});
Personally, I think we probably want that test to pass. But there are some questions about how the ordering priority interacts with the global vs. local priority. I imagine
For example, would you expect this test to pass? I would. (Currently it fails, for the same reason as in the test above.) test.only('global negations in schema should be overriden by global match coming after it, but not by local match', async () => {
const mockWatcher = await setupMockWatcher({
filepath: './foo/some-config.ts',
config: {
schema: './foo/something.ts',
documents: ['!./foo/exclude-all-but-apple/*.graphql', './foo/exclude-all-but-apple/apple.graphql'],
generates: {
['./foo/some-output.ts']: {
documents: [
'./foo/exclude-all-but-apple/red-fruit.graphql'
],
},
},
},
});
expect(mockWatcher.watchDirectory).toBe(join(process.cwd(), 'foo'));
await assertBuildTriggers(mockWatcher, {
shouldTriggerBuild: ['./foo/exclude-all-but-apple/apple.graphql'],
shouldNotTriggerBuild: [
'./foo/exclude-all-but-apple/orange.graphql',
'./foo/exclude-all-but-apple/banana.graphql',
// This is excluded by global documents, so even though it's included locally, we exclude it
'./foo/exclude-all-but-apple/red-fruit.graphql',
],
});
});BUT.... what about this one? test.only('global negations in schema should be overriden by global match coming after it, but not by local match', async () => {
const mockWatcher = await setupMockWatcher({
filepath: './foo/some-config.ts',
config: {
schema: './foo/something.ts',
documents: ['!./foo/exclude-all-but-apple/*.graphql', './foo/exclude-all-but-apple/apple.graphql'],
generates: {
['./foo/some-output.ts']: {
documents: [
+ // But what if we repeat the exclusion here? Should that "reset" the global,
+ // and allow us to override it with red-fruit locally, so that red-fruit will be a match?
+ '!./foo/exclude-all-but-apple/*.graphql',
'./foo/exclude-all-but-apple/red-fruit.graphql'
],
},
},
},
});
expect(mockWatcher.watchDirectory).toBe(join(process.cwd(), 'foo'));
await assertBuildTriggers(mockWatcher, {
shouldTriggerBuild: [
'./foo/exclude-all-but-apple/apple.graphql',
+ // Should this trigger build????
+ './foo/exclude-all-but-apple/red-fruit.graphql'
],
shouldNotTriggerBuild: [
'./foo/exclude-all-but-apple/orange.graphql',
'./foo/exclude-all-but-apple/banana.graphql',
- // This is excluded by global documents, so even though it's included locally, we exclude it
- './foo/exclude-all-but-apple/red-fruit.graphql',
],
});
});Personally, I think that test should probably fail, and if you want something like this (to include Let me know what you think. It's easy to add this behavior, but we just need to decide what the expectation is (which ideally should match the behavior of |
After pondering this a bit more, I now think that it's not important for us to consider order of rules, within a stanza or otherwise. What is important is to give users a way to "default exclude everything, but then include this other thing." And we do give them that, by offering both local and global priority, where one (global) takes priority over the other. This is no different than So, it should be possible to take advantage of the fact that test.only('exclude all, but include this one thing', async () => {
const mockWatcher = await setupMockWatcher({
filepath: './foo/some-config.ts',
config: {
schema: './foo/something.ts',
watch: ['./foo/exclude-all-but-apple/apple.graphql'],
generates: {
['./foo/some-output.ts']: {
documents: ['!./foo/exclude-all-but-apple/*.graphql'],
},
['./foo/some-other-output.ts']: {
documents: ['!./foo/exclude-all-but-apple/*.graphql'],
},
},
},
});
expect(mockWatcher.watchDirectory).toBe(join(process.cwd(), 'foo'));
await assertBuildTriggers(mockWatcher, {
shouldTriggerBuild: ['./foo/exclude-all-but-apple/apple.graphql'],
shouldNotTriggerBuild: [
'./foo/exclude-all-but-apple/orange.graphql',
'./foo/exclude-all-but-apple/banana.graphql',
'./foo/exclude-all-but-apple/red-fruit.graphql',
],
});
});But there are a few inconvenient things about this:
Point is, it's possible to use the precedence order of global vs. local and watch vs. others to accomplish this pattern of "exclude all, but then make an exception for this one thing." And that's what's important, IMO. However, I have no idea if this is a breaking change given what other people's configs look like, and if they were relying on order of precedence beforehand. Of course, it would only be a breaking change for watch mode, which is not the end of the world. But maybe we should add some heuristics to detect rules that look like they might depend on ordering precedence, and print a notice that it might not work as expected, and to use |
|
@milesrichardson any idea on the failing test. It says timeouts 🤔 idk if increasing timeout will make it work, probably there is a memory leak to look at with thew new watch stuff |
yes, the tests started failing when I switched to using If I revert that commit that switched to |
… by micromatch patterns This commit adds `ExcludeQueryAlpha` and `ExcludeQueryBeta`, and then checks them in along with the result of running `yarn generate:examples`, since this should be the baseline of the unmodified codegen. A separate commit will add some new outputs to the `star-wars` example that alternately exclude and include these files, in order to test pattern matching works with both generating and watching the files.
… baseline testing This commit adds a baseline for testing pattern matching with contravariant negations between two config stanzas. That is, the `types.excludeQueryAlpha.ts` target produces an output that includes `ExcludeQueryBeta` but not `ExcludeQueryAlpha`, and the `types.excludeQueryBeta.ts` target produces an output that includes `ExcludeQueryAlpha` but not `ExcludeQueryBeta`. Manual verification shows that the output files correctly exclude these types when running generation, and so they can be checked in as a baseline for later tests to detect a regression if that changes. Otherwise, they'll be used primarily for testing `yarn watch:examples`, which we expect will adhere to the same exclusion rules during _generation_, but which we need to make ure also adheres to those rules during watch mode, while filtering file change events to trigger (or not trigger) rebuilds.
Add a file in `dev-test-outer-dir` which is excluded by the pattern that's part of a pattern set otherwise matching `*.graphql` files in that directory. This way, we can check that touching this file does not ever trigger a rebuild during watch mode (and also note that this commits shows it's not included in any output during generation). In fact, the bug we're fixing in watch mode is so sensitive that we can simply run `git status` to trigger a change to the Git `index.lock`, but this way at least we have a real `*.graphql` file we can touch and watch for avoidance of rebuild triggers.
…ready absolute Parcel Watcher always provides `path` as an absolute path (see the readme for the package), and we were incorrectly concatenating it with absolute path `watchDirectory` (and prior to dotansimha#9270, with `process.cwd()`, which is also an absolute path). This removes the concatenation, which should fix a potential bug when comparing to `config.configFilePath`. However, it also means that since dotansimha#9050, we've been passing an absolute path to the `onWatchTriggered` hook, which may or may not be expected by downstream user code. This commit leaves that unchanged, and we still pass it an absolute path.
Implement the algorithm for deciding when to trigger a rebuild during watch mode, in such a way that precedence (both in terms of global config preceding per-target config, and in terms of watch patterns preceding document/schema patterns) is respected, and negations are evaluated within their local groups. This algorithm is described and diagrammed here: dotansimha#9270 (comment)
Make `createWatcher` more testable, by returning a `stopWatching` callback that can be called to stop the otherwise infinitely running watcher. Implement it by adding an abort signal handler to the existing process.exit and SIGINT handlers, to call `shutdown()` in the same way.
Create mocking infrastructure for the watcher, including mocking `@parcel/watcher`, so that tests can dispatch file change events and spy on the subscription callback.
This commit implements graceful shutdown for the watcher, including when cancelled via an AbortSignal. It also includes a polyfill for AbortController, which is necessary on Node v14 (at least without passing any experimental flags). Shutdown events happen in logical order, and as long as the caller does `await stopRunning()`, it's guaranteed the promise will only resolve after the watch server has terminated completely.
…red rebuild Add a helper function that takes a mock watcher, a list of files that should trigger rebuild, and a list of files that should not trigger rebuild, and then automatically tests all of them. Call the `expect()` functions with an assertion helper that ensures the stack trace shows the relevant functon call, and also adds a formatted prelude clearly showing which path was expected to trigger or not trigger a rebuild, and what happened instead.
…tchDirectory ParcelWatcher expects the `ParcelWatcher.Options["ignore"]` array to include either absolute paths, or relative paths that are relative to the `watchDirectory`. This commit fixes a bug where the paths of ignored files (i.e., output directories from stanzas with presetConfig) were relative to `process.cwd()`, which may differ from `watchDirectory`
…factor Add mocks, assertion helpers, and error formatters for testing the value of paths and globs passed to the `options["ignore"]` argument of `parcelWatcher.subscribe()`. Note that we can't properly mock the behavior of this, because ParcelWatcher implements its ignore logic in C++, so the best we can do is make assertions on the argument it receives. This commit also refactors all of the watch mode testing infrastructure to be in a `tests/watch-helpers` directory.
…relative to" The "relative from" nomenclature matches how NodeJS docs refer to the concept of one path being relative from another, and in the case of changes to comments, also matches the actual variable names which were already using `from` instead of `to`
When picking `watchDirectory` by finding the "longest common prefix directory"
of all paths and glob patterns, only include affirmative (non-negated)
patterns. Otherwise, a rule like `!**/never.graphql` will cause
the `watchDirectory` to always be `process.cwd()`, because the
result of `mm.scan("!**/never.graphql").base` is `""`.
…st cases Remove `pathMatches` shortcut in favor of explicitly checking for negation after finding an affirmative match, because it turns out this doesn't work. Now the code exactly matches the flow chart. The reason for this change is that micromatch does not give negation priority when there is an exact affirmative match, but we expect it to (because in our case, negations _always_ take precedence). The micromatch behavior makes sense when you're considering one set of patterns, because why would you both include and exclude an exact path? But we have the additional complexity of mixing global and local pattern sets, so we can naturally arrive at a situation where a set includes both an exact negated match, and an exact affirmative match. In this case we still want to negate it. Without this fix, the logic was not working as expected, because a global negation was being ignored when there was a local inclusion.
…ed one For each individual assertion we were making in the big combined config, isolate it into a single test that captures the important aspect of it, and also add some more specificity to the logic of some of the assertions, which is much easier to see when isolated within a single test. This should make it easier to identify the source of any future regressions. Still, keep the existing test with the big combined config, because it can't hurt. But change "foo" to "fuzz" in that test, just to create different strings in case someone wants to grep for a path in a failing individual test.
There is no need for a poly/pony-fill.
…ch`" This reverts commit 7a374c2. There is some issue with the implementation of `AbortController` in `@whatwg-node/fetch` which causes all the tests to timeout (presumably because the `AbortSignal` is either not being emitted or received).
milesrichardson
force-pushed
the
watch-negated-patterns
branch
from
7a374c2
to
96f3cfe
Compare
|
I just reverted the commit using Let's see if tests pass again. It's possible there will be a failure in a flaky test as observed before, but as mentioned in that comment, I'm pretty sure it's unrelated to this MR. EDIT: All tests passing again. If anything should be addressed in this PR, I think it should be the discussion about mutual exclusivity and ordering priority. |
|
@saihaj @ardatan @eddeee888 @beerose can you please take a look? :) |
dotansimha
requested review from
beerose and
n1ru4l
and removed request for
dotansimha
|
@milesrichardson Thank you so much for this pull request. It feels like you put in a lot of thought and effort, which I highly appreciate! My only minor concern is regarding the ruling order. In my opinion, the order should matter. But I am happy to get convinced otherwise. Do you know how other CLI tools are handling this? AFAIK in a Happy to get this merged soon! We can also keep the |
|
@n1ru4l See #9275 (comment) for my latest thoughts on whether order is important. My thinking is that it's not order of precedence that matters, but giving the user a way to say "exclude everything, except for this one thing" - which we do allow them to do by using the precedence of global vs. local. Once you introduce per-stanza ordering rules, the issue becomes how the differing orders across stanzas interact with each other (see #9275 (comment)). What I would like to know is what the behavior of non-watch mode is when it comes to ordering precedence. |
STATUS (Apr 8): ✅ Ready for review, rebased, description updated, self-review complete
UPDATE (Apr 8): This PR is ready for review (and IMO, ready to merge ;)). The code is rebased on latest
master. I added a lot of test cases (seewatcher.spec.ts), which I believe cover all of the specification in terms of the behavior we want. These tests surfaced some bugs and I fixed them. Also, I updated the description in this PR to reflect the latest information about testing.UPDATE (Apr 7): I've added a bunch of test infrastructure code, namely: making the watcher cancellable, mocking ParcelWatcher and event dispatching/handling, adding assertion helpers for creating data-driven assertions with DRY test code, and formatting test error output to clearly show what did/didn't trigger. The code in this PR is now relatively stable, and all I intend to do is add more test cases. You could start a review if you'd like.
DRAFT (Apr 5): The implementation is working as far as I can tell, and it's an improvement over the status quo of very aggressive rebuild triggers during watch mode (even just running
git statustriggers a rebuild because it changes.git/index.lock). But ideally I want to add some unit tests before merging this, although I'm a bit tired now so we'll see.Description
This PR implements an algorithm for deciding which file change events should trigger a rebuild during watch mode. It respects the precedence rules, both in terms of "global' (top level) config preceding "local" (per-output target) config, and in terms of "watch patterns" (both global and local) preceding "document" and "schema" patterns.
The algorithm assumes that the config file defines at least one output target. The basic idea is that it creates a
matcherfor each local config, and then on a file change event, it calls those matchers until it finds one that returnstrue. The specification of the algorithm ensures that as long as there is at least one output target, then all rules of precedence and negation will be respected.The algorithm is described in this comment of #9270. For convenience, I'm including the diagram of the flow chart here as well (to read it, start from the top left):
SVG diagram of flowchart describing the algorithm
🔗 Raw link to SVG image
PNG diagram of flowchart describing the algorithm
🔗 Raw link to PNG image
Related #9270
Type of change
How Has This Been Tested?
Unit Tests
I added 500 lines of unit tests, and also made
watchertestable, namely by: making it cancellable, mocking ParcelWatcher, and adding assertion helpers for testing whether or not a change event triggers a build. Seewatcher.spec.ts(note it's collapsed in the diff view) for all the test cases, which I hope you'll find easily readable, thanks to the assertion helpers. :)To run:
Here's a picture of those tests passing, including the names of the cases which should give you an idea of what's covered:
Crude Smoke Tests
I've tested it locally, and added some configs to
dev-test/codegen.tsthat could impact watch mode. You can run watch mode like this:Then you can try changing a few files:
Here's a video of that:
codegen-watch-server.mp4
Test Environment:
@graphql-codegen/...: This PRChecklist:
Further comments
There is a lot of code here, but don't be alarmed! It's not as bad as it looks. 😄 I also did a self-review and left a bunch of comments, but none of them are actionable; I just wanted to provide some background info on various code choices, but you can just ignore them too.
If you only review two files, these are the most important:
patterns.ts(implements the algorithm in the flow chart)watcher.spec.ts(describes all the test cases)If you have a question about the behavior
Check whether the case is covered in
watcher.spec.ts. If it's not, it should be really easy to add one. All you need to do is describe a config, and then list which changes you think should or should not trigger a build! 😄 Please let me know if I missed any corner cases in thewatcher.spec.ts.Here is the gist of the changes:
patterns.tsmakeShouldRebuildfunction (which creates theshouldRebuildcallback thatwatcher.tscalls) (please see--watchmode does not filter out file change events that are negated as part of a set of patterns #9270 for longer discussion/explanation of the algorithm)watcher.spec.tswatchDirectory(testing behavior of--watchOnly watches files belowprocess.cwd(), even though some paths may be outside of it #9266)dev-test/codegen.tsyarn watch:exampleswatcher.tsshouldRebuildfrompatterns.tsParcelWatcher.Options["ignore"]are relative fromwatchDirectory(as expected by ParcelWatcher), not fromcwdfullPathto just usepathwhen passed toonWatchTriggeredtests/watcher-test-helperswatcher-test-helpers/setup-mock-watcher.tsParcelWatcherthat returns functions for making assertions on it:ParcelWatcher.Options.ignore) passed by implementing code toParcelWatcher.subscribe()onWatchTriggeredlifecycle hook, which is called when a build is triggereddispatchChange(someFilePath)events to the mock watcher, so they can then make assertions on the subscribe callback andonWatchTriggeredcallback, which together can be used to see if a file change did or did not trigger a buildwatcher-test-helpers/assert-watcher-build-triggers.tsassertBuildTriggers()helper function, which takes as arguments: the mock watcher, a list of paths that should trigger build, a list of paths that should not trigger build, a list of paths that should be passed toParcelWatcher.Options.ignore, and a list of globs that should be passed toParcelWatcher.Options.ignorewatch-test-helpers/format-watcher-assertion-errors.tsParcelWatcher.Options.ignore, it also pretty prints tables showing what paths were passed toignoreand what was expected to be there but not found.Here are some examples of what the formatting looks like for test failures:
Expected file to trigger build, but it didn't
Expected file NOT to trigger build, but it did
Expected path to be passed to ParcelWatcher.Options.ignore, but it was not
See: ParcelWatcher
ignoreoptionExpected glob to be passed to ParcelWatcher.Options.ignore, but it was not
See: ParcelWatcher
ignoreoption