Merge pull request #84 from rtablada/error-response

Consider 4xx and 5xx status as error instead of trying to render MD

.eslintrc.cjs

@@ -28,7 +28,7 @@ module.exports = {
       },
     },
     {
-      files: ['src/browser/**/*', '**/*.test.ts'],
+      files: ['src/browser/**/*', '**/*.test.ts', 'src/plugins/markdown-pages/types.ts'],
       rules: {
         'n/no-unpublished-import': 'off',
         'n/no-missing-import': 'off',

docs-app/app/templates/page.gjs

@@ -11,9 +11,10 @@ export default Route(
   <template>
     <Page>
       <:error as |error|>
-        <div style="border: 1px solid red; padding: 1rem;">
+        <div style="border: 1px solid red; padding: 1rem;" data-page-error>
           {{error}}
         </div>
+        {{(removeLoader)}}
       </:error>
 
       <:success as |Prose|>

docs-app/public/docs/plugins/kolay.md

@@ -5,8 +5,8 @@ Kolay requires some build-time static analysis to function.
 `kolay(...)` is the only required plugin. This generates the navigation and information about how Kolay's runtime code will fetch the markdown documents deployed with the app's static assets. Optionally, if a list of packages is provided, apiDocs will be generated from your library's type declarations. Rendering these api docs uses the [Signature Components][ui-signature] or [`APIDocs`][ui-apiDocs] components.
 
 [plugin-kolay]: /plugins/kolay.md
-[ui-signature]: /Runtime/components/component-signature.md
-[ui-apiDocs]: /Runtime/components/api-docs.md
+[ui-signature]: /Runtime/docs/component-signature.md
+[ui-apiDocs]: /Runtime/docs/api-docs.md
 
 Usage in embroider / webpack:
 

docs-app/public/docs/usage/rendering-pages.md

@@ -21,6 +21,7 @@ export default Route(
         <div style="border: 1px solid red; padding: 1rem;">
           {{error}}
         </div>
+        {{(removeLoader)}}
       </:error>
 
       <:success as |prose|>

docs-app/tests/docs-app/errors-test.gts

@@ -0,0 +1,55 @@
+import { renderSettled } from '@ember/renderer';
+import { click, settled, visit } from '@ember/test-helpers';
+import { module, test } from 'qunit';
+import { setupApplicationTest } from 'ember-qunit';
+
+module('Errors', function (hooks) {
+  setupApplicationTest(hooks);
+
+  module('Not found', function () {
+    test('not in any manifest / group', async function (assert) {
+      await visit('/usage/does-not-exist.md');
+
+      assert.dom().doesNotContainText(`Cannot GET`, 'does not directly expose errors from fetch');
+      assert.dom().doesNotContainText(`null`, 'does not incorrectly render the error');
+
+      assert.dom(`[data-page-error]`).containsText(`Page not found for path`);
+      assert.dom(`[data-page-error]`).containsText(`/usage/does-not-exist`);
+      assert.dom(`[data-page-error]`).containsText(`Using group: root`);
+    });
+
+    test(`can recover after an error`, async function (assert) {
+      await visit('/usage/does-not-exist.md');
+
+      assert.dom(`[data-page-error]`).containsText(`Page not found for path`);
+
+      await click(`a[href="/usage/setup.md"]`);
+
+      assert.dom().doesNotContainText(`Page not found for path`);
+    });
+
+    test(`attempting to visit a route that doesn't exist in the current group, but does exist in another group`, async function (assert) {
+      await visit(`/Runtime/docs/api-docs.md`);
+
+      assert.dom().doesNotContainText(`Page not found for path`);
+    });
+
+    test('the error does not flash between known pages rendering', async function (assert) {
+      visit(`/Runtime/docs/api-docs.md`);
+
+      await renderSettled();
+      assert.dom('[data-page-error]').doesNotExist();
+
+      await settled();
+      assert.dom('[data-page-error]').doesNotExist();
+
+      visit(`/Runtime/util/logs.md`);
+
+      await renderSettled();
+      assert.dom('[data-page-error]').doesNotExist();
+
+      await settled();
+      assert.dom('[data-page-error]').doesNotExist();
+    });
+  });
+});

package.json

@@ -80,7 +80,7 @@
     "lint:prettier:fix": "prettier . --write",
     "lint:published-types": "attw --pack",
     "start:all": "concurrently 'pnpm --filter ./ui start' 'pnpm --filter ./docs-app start' --names ui,docs",
-    "build": "pnpm prepack",
+    "build": "(cd ui && pnpm build) && pnpm prepack",
     "build:declarations": "tsc --declaration",
     "_syncPnpm": "pnpm sync-dependencies-meta-injected",
     "start:typedoc": "typedoc --options ./typedoc.config.json --watch",

ui/package.json

@@ -105,6 +105,7 @@
       "./services/kolay/compiler/import-map.js": "./dist/_app_/services/kolay/compiler/import-map.js",
       "./services/kolay/compiler/reactive.js": "./dist/_app_/services/kolay/compiler/reactive.js",
       "./services/kolay/docs.js": "./dist/_app_/services/kolay/docs.js",
+      "./services/kolay/request.js": "./dist/_app_/services/kolay/request.js",
       "./services/kolay/selected.js": "./dist/_app_/services/kolay/selected.js",
       "./services/kolay/types.js": "./dist/_app_/services/kolay/types.js"
     }

ui/src/services/kolay/docs.ts

@@ -204,6 +204,23 @@ export default class DocsService extends Service {
 
     return group;
   };
+
+  /**
+   * Will return false if a url doesn't exist in any group,
+   * or the name of the group that contains the page if the url does exist.
+   */
+  groupForURL = (url: string): false | string => {
+    for (let groupName of this.availableGroups) {
+      let group = this.groupFor(groupName);
+      let page = group.list.find((page) => page.path === url);
+
+      if (page) {
+        return groupName;
+      }
+    }
+
+    return false;
+  };
 }
 
 /**

ui/src/services/kolay/request.ts

@@ -0,0 +1,64 @@
+import { cached } from '@glimmer/tracking';
+import { service } from '@ember/service';
+
+import { use } from 'ember-resources';
+import { keepLatest } from 'reactiveweb/keep-latest';
+import { RemoteData } from 'reactiveweb/remote-data';
+
+import type DocsService from './docs';
+
+export const OUTPUT_PREFIX = `/docs/`;
+export const OUTPUT_PREFIX_REGEX = /^\/docs\//;
+
+/**
+ * With how data is derived here, the `fetch` request does not execute
+ * if we know ahead of time that the fetch would fail.
+ * e.g.: when the URL is not declared in the manifest.
+ *
+ * The `fetch` only occurs when `last` is accessd.
+ * and `last` is not accessed if `doesPageExist` is ever false.
+ */
+export class MDRequest {
+  constructor(private urlFn: () => string) {}
+
+  @service('kolay/docs') declare docs: DocsService;
+
+  /**
+   * TODO: use a private property when we move to spec-decorators
+   */
+  @use last = RemoteData<string>(this.urlFn);
+
+  @use lastSuccessful = keepLatest({
+    value: () => this.#lastValue,
+    when: () => this.hasError,
+  });
+
+  get hasError() {
+    if (!this._doesPageExist) return true;
+
+    /**
+     * Can't have an error if we haven't made a request yet
+     */
+    if (!this.last.status) return false;
+
+    return this.last.status >= 400;
+  }
+
+  /**
+   * TODO: use a private property when we move to spec-decorators
+   */
+  @cached
+  private get _doesPageExist() {
+    let url = this.urlFn();
+    let pagePath = url.replace(OUTPUT_PREFIX_REGEX, '/');
+    let group = this.docs.groupForURL(pagePath);
+
+    return Boolean(group);
+  }
+
+  get #lastValue() {
+    if (!this._doesPageExist) return '';
+
+    return this.last.value;
+  }
+}

ui/src/services/kolay/selected.ts

@@ -2,11 +2,11 @@ import Service, { service } from '@ember/service';
 
 import { use } from 'ember-resources';
 import { keepLatest } from 'reactiveweb/keep-latest';
-import { RemoteData } from 'reactiveweb/remote-data';
+import { link } from 'reactiveweb/link';
 
 import { Compiled } from './compiler/reactive.ts';
+import { MDRequest } from './request.ts';
 
-import type CompilerService from './compiler';
 import type DocsService from './docs';
 import type { Page } from './types';
 import type RouterService from '@ember/routing/router-service';
@@ -25,10 +25,25 @@ import type RouterService from '@ember/routing/router-service';
 
 const firstPath = '/1-get-started/intro.md';
 
+/**
+ * Sort of like an ember-concurrency task...
+ * if we ignore concurrency and only care about the states of the running function
+ * (and want automatic invocation based on derivation)
+ */
+class Prose {
+  constructor(private docFn: () => string | null) {}
+
+  @use last = Compiled(this.docFn);
+
+  @use lastSuccessful = keepLatest({
+    value: () => this.last.component,
+    when: () => !this.last.isReady,
+  });
+}
+
 export default class Selected extends Service {
   @service declare router: RouterService;
   @service('kolay/docs') declare docs: DocsService;
-  @service('kolay/compiler') declare compiler: CompilerService;
 
   /*********************************************************************
    * These load the files from /public and handle loading / error state.
@@ -37,8 +52,12 @@ export default class Selected extends Service {
    * be cancelled if it was still pending.
    *******************************************************************/
 
-  @use proseFile = RemoteData<string>(() => `/docs${this.path}.md`);
-  @use proseCompiled = Compiled(() => this.proseFile.value);
+  @link request = new MDRequest(() => `/docs${this.path}.md`);
+  @link compiled = new Prose(() => this.request.lastSuccessful);
+
+  get proseCompiled() {
+    return this.compiled.last;
+  }
 
   /*********************************************************************
    * This is a pattern to help reduce flashes of content during
@@ -47,11 +66,9 @@ export default class Selected extends Service {
    * we can, and only swap out the old data when the new data is done loading.
    *
    ********************************************************************/
-
-  @use prose = keepLatest({
-    value: () => this.proseCompiled.component,
-    when: () => !this.proseCompiled.isReady,
-  });
+  get prose() {
+    return this.compiled.lastSuccessful;
+  }
 
   /**
    * Once this whole thing is "true", we can start
@@ -62,9 +79,13 @@ export default class Selected extends Service {
   }
 
   get hasError() {
-    return Boolean(this.proseCompiled.error);
+    return Boolean(this.proseCompiled.error) || this.request.hasError;
   }
   get error() {
+    if (!this.page) {
+      return `Page not found for path ${this.path}. (Using group: ${this.docs.currentGroup.name})`;
+    }
+
     return String(this.proseCompiled.error);
   }