CODEGEN-866 [CLI] Fix generated files disappearing in watch mode (#10468)

* Fix typing

* Update typing for devX

* Update watcher to not write output on error, and add tests

* Add changeset

* Update tests to be less flaky

Author: eddeee888

.changeset/solid-oranges-shave.md

@@ -0,0 +1,11 @@
+---
+'@graphql-codegen/cli': patch
+---
+
+In watch mode, do not write output on failure
+
+Previously, on partial or full failure, watch mode still write to output. However, since the output'd be an empty array, it will then call `removeStaleFiles` internally to remove all previously generated files.
+
+This patch puts a temporary fix to avoid writing output on any failure to fix the described behaviour.
+
+This also means the `config.allowPartialOutputs` does not work in watch mode for now.

packages/graphql-codegen-cli/src/generate-and-save.ts

@@ -15,7 +15,14 @@ const hash = (content: string): string => createHash('sha1').update(content).dig
 export async function generate(
   input: CodegenContext | (Types.Config & { cwd?: string }),
   saveToFile = true
-): Promise<Types.FileOutput[] | any> {
+): Promise<
+  | Types.FileOutput[]
+  /**
+   * When this function runs in watch mode, it'd return an empty promise that doesn't resolve until the watcher exits
+   * FIXME: this effectively makes the result `any`, which loses type-hints
+   */
+  | any
+> {
   const context = ensureContext(input);
   const config = context.getConfig();
   await context.profiler.run(() => lifecycleHooks(config.hooks).afterStart(), 'Lifecycle: afterStart');
@@ -43,7 +50,7 @@ export async function generate(
 
   const recentOutputHash = new Map<string, string>();
 
-  async function writeOutput(generationResult: Types.FileOutput[]) {
+  async function writeOutput(generationResult: Types.FileOutput[]): Promise<Types.FileOutput[]> {
     if (!saveToFile) {
       return generationResult;
     }

packages/graphql-codegen-cli/src/utils/watcher.ts

@@ -79,7 +79,21 @@ export const createWatcher = (
       if (!isShutdown) {
         executeCodegen(initialContext)
           .then(
-            ({ result }) => onNext(result),
+            ({ result, error }) => {
+              // FIXME: this is a quick fix to stop `onNext` (writeOutput) from
+              // removing all files when there is an error.
+              //
+              // This is because `removeStaleFiles()` will remove files if the
+              // generated files are different between runs. And on error, it
+              // returns an empty array i.e. will remove all generated files from
+              // the previous run
+              //
+              // This also means we don't have config.allowPartialOutputs in watch mode
+              if (error) {
+                return;
+              }
+              onNext(result);
+            },
             () => Promise.resolve()
           )
           .then(() => emitWatching(watchDirectory));
@@ -202,7 +216,14 @@ export const createWatcher = (
   stopWatching.runningWatcher = new Promise<void>((resolve, reject) => {
     executeCodegen(initialContext)
       .then(
-        ({ result }) => onNext(result),
+        ({ result, error }) => {
+          // TODO: this is the initial run, the logic here mimics the above watcher logic.
+          // We need to check whether it's ok to deviate between these two.
+          if (error) {
+            return;
+          }
+          onNext(result);
+        },
         () => Promise.resolve()
       )
       .then(() => runWatcher(abortController.signal))

packages/graphql-codegen-cli/tests/watcher-test-helpers/assert-watcher-build-triggers.ts

@@ -1,5 +1,5 @@
 import { join, isAbsolute, relative, resolve, sep } from 'path';
-import { Options } from '@parcel/watcher';
+import type { Options } from '@parcel/watcher';
 import isGlob from 'is-glob';
 import type { Mock } from 'vitest';
 
@@ -22,7 +22,7 @@ interface MockWatcher {
 
 /**
  * Helper function for asserting that multiple paths did or did not trigger a build,
- * and for asserting the values of paths and globs passed to {@link ParcelWatcher.Options}`["ignore"]`
+ * and for asserting the values of paths and globs passed to {@link Options}`["ignore"]`
  */
 export const assertBuildTriggers = async (
   mockWatcher: MockWatcher,
@@ -224,7 +224,7 @@ const assertParcelWouldIgnoreGlob = (
 };
 
 /**
- * Given a path, and the `ignore` option passed to the mocked {@link ParcelWatcher.Options},
+ * Given a path, and the `ignore` option passed to the mocked {@link Options},
  * assert that ParcelWatcher "would" ignore the path if given it as part of the ignore option.
  *
  * Note that ParcelWatcher expects paths relative from the watchDirectory, but

packages/graphql-codegen-cli/tests/watcher.spec.ts

@@ -10,6 +10,8 @@ const unsubscribeMock = vi.fn();
 const subscribeMock = vi.fn();
 let subscribeCallbackMock: Mock<SubscribeCallback>;
 
+// FIXME: this mocks out the main functionality which is triggering the codegen
+// This is not great because we cannot test the actual watch functionality
 vi.mock('@parcel/watcher', () => ({
   subscribe: subscribeMock.mockImplementation((watchDirectory: string, subscribeCallback: SubscribeCallback) => {
     subscribeCallbackMock = vi.fn(subscribeCallback);
@@ -19,13 +21,16 @@ vi.mock('@parcel/watcher', () => ({
   }),
 }));
 
-const setupMockWatcher = async (codegenContext: ConstructorParameters<typeof CodegenContext>[0]) => {
-  const { stopWatching } = createWatcher(new CodegenContext(codegenContext), async () => Promise.resolve([]));
+const setupMockWatcher = async (
+  codegenContext: ConstructorParameters<typeof CodegenContext>[0],
+  onNext: Mock = vi.fn().mockResolvedValue([])
+) => {
+  const { stopWatching } = createWatcher(new CodegenContext(codegenContext), onNext);
 
   const dispatchChange = async (path: string) => subscribeCallbackMock(undefined, [{ type: 'update', path }]);
 
   // createWatcher doesn't set up subscription immediately, so we wait for a tick before continuing
-  await new Promise(resolve => setTimeout(resolve, 10));
+  await new Promise(resolve => setTimeout(resolve, 100));
 
   return { stopWatching, dispatchChange };
 };
@@ -771,4 +776,53 @@ describe('Watch targets', () => {
       }
     );
   });
+
+  test('it does not call onNext on error', async () => {
+    vi.spyOn(fs, 'access').mockImplementation(() => Promise.resolve());
+    const onNextMock = vi.fn();
+
+    const schema = /* GraphQL */ `
+      type Query {
+        me: User
+      }
+
+      type User {
+        id: ID
+      }
+    `;
+    const document = /* GraphQL */ `
+      query {
+        me {
+          id
+          zzz # Error here
+        }
+      }
+    `;
+
+    const { stopWatching } = await setupMockWatcher(
+      {
+        filepath: './foo/some-config.ts',
+        config: {
+          hooks: { onWatchTriggered: vi.fn() },
+          schema,
+          documents: document,
+          generates: {
+            ['./foo/some-output.ts']: {
+              plugins: ['typescript'],
+            },
+          },
+        },
+      },
+      onNextMock
+    );
+
+    // Because document has error, onNext shouldn't be called
+    expect(onNextMock).not.toHaveBeenCalled();
+
+    // Wait a tick for stopWatch to be set up correctly, before calling it
+    await new Promise(resolve => setTimeout(resolve, 100));
+    await stopWatching();
+  });
+
+  test.todo('on watcher subsequent codegen run, it does not call onNext on error');
 });