refactor(router-lite): query in fragment when using useUrlFragmentHas…

…h option (#1794) * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * chore(router-lite): cleanup * refactor(router-lite): url serializer * refactor(router-lite): url parser * refactor(router-lite): url parser stringify * refactor(router-lite): e2e tests * test(router-lite): query string in hash * refactor(router-lite): nested fragment * refactor: router-lite * fix(router-lite): e2e build * chore(router-lite): post-review changes
aurelia · Jul 14, 2023 · a1ca36d · a1ca36d
1 parent e8e39a9
commit a1ca36d
Show file tree

Hide file tree

Showing 12 changed files with 642 additions and 470 deletions.
diff --git a/packages/__e2e__/router-lite/src/router-event-logger-service.ts b/packages/__e2e__/router-lite/src/router-event-logger-service.ts
@@ -1,4 +1,4 @@
-import { IRouterEvents } from '@aurelia/router-lite';
+import { IRouterEvents, pathUrlParser } from '@aurelia/router-lite';
 import { DI, IDisposable } from 'aurelia';
 
 export const IRouterEventLoggerService = /*@__PURE__*/DI.createInterface<IRouterEventLoggerService>('ISomeService', x => x.singleton(RouterEventLoggerService));
@@ -12,16 +12,16 @@ export class RouterEventLoggerService implements IDisposable {
         this.log.push(`${event.name} - '${event.url}'`);
       }),
       events.subscribe('au:router:navigation-start', (event) => {
-        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl()}'`);
+        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}'`);
       }),
       events.subscribe('au:router:navigation-end', (event) => {
-        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl()}'`);
+        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}'`);
       }),
       events.subscribe('au:router:navigation-cancel', (event) => {
-        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl()}' - ${String(event.reason)}`);
+        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}' - ${String(event.reason)}`);
       }),
       events.subscribe('au:router:navigation-error', (event) => {
-        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl()}' - ${String(event.error)}`);
+        this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}' - ${String(event.error)}`);
       }),
     ];
   }

diff --git a/packages/__tests__/router-lite/ast.spec.ts b/packages/__tests__/router-lite/ast.spec.ts
diff --git a/packages/__tests__/router-lite/events.spec.ts b/packages/__tests__/router-lite/events.spec.ts
@@ -1,5 +1,5 @@
 import { Class, DI, IDisposable, LogLevel, noop } from '@aurelia/kernel';
-import { IRouter, IRouterEvents, Params, RouterConfiguration, route } from '@aurelia/router-lite';
+import { IRouter, IRouterEvents, Params, pathUrlParser, RouterConfiguration, route } from '@aurelia/router-lite';
 import { AppTask, Aurelia, customElement } from '@aurelia/runtime-html';
 import { TestContext, assert } from '@aurelia/testing';
 import { TestRouterConfiguration } from './_shared/configuration.js';
@@ -39,16 +39,16 @@ describe('router-lite/events.spec.ts', function () {
     public constructor(@IRouterEvents events: IRouterEvents) {
       this.subscriptions = [
         events.subscribe('au:router:navigation-start', (event) => {
-          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, false)}'`);
+          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}'`);
         }),
         events.subscribe('au:router:navigation-end', (event) => {
-          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, false)}'`);
+          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}'`);
         }),
         events.subscribe('au:router:navigation-cancel', (event) => {
-          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, false)}' - ${String(event.reason)}`);
+          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}' - ${String(event.reason)}`);
         }),
         events.subscribe('au:router:navigation-error', (event) => {
-          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, false)}' - ${String(event.error)}`);
+          this.log.push(`${event.name} - ${event.id} - '${event.instructions.toUrl(false, pathUrlParser)}' - ${String(event.error)}`);
         }),
       ];
     }

diff --git a/packages/__tests__/router-lite/smoke-tests.spec.ts b/packages/__tests__/router-lite/smoke-tests.spec.ts
@@ -3654,6 +3654,152 @@ describe('router-lite/smoke-tests.spec.ts', function () {
       await au.stop(true);
     });
   }
+
+  it('querystring is added to the fragment when hash-based routing is used', async function () {
+    @customElement({ name: 'c-1', template: `c1 params: \${id} query: \${query} fragment: \${fragment}` })
+    class C1 implements IRouteViewModel {
+
+      private id: string;
+      private query: string;
+      private fragment: string;
+
+      public loading(params: Params, node: RouteNode): void | Promise<void> {
+        this.id = params.id;
+        this.query = node.queryParams.toString();
+        this.fragment = node.fragment;
+      }
+    }
+
+    @route({ routes: [{ id: 'c1', path: 'c1/:id', component: C1 }] })
+    @customElement({ name: 'app', template: '<a load="route: c1; params.bind: {id: 42, foo: \'bar\'}"></a><au-viewport></au-viewport>' })
+    class App { }
+
+    const { host, container } = await start({ appRoot: App, useHash: true });
+
+    const anchor = host.querySelector('a');
+    assert.match(anchor.href, /#\/c1\/42\?foo=bar$/);
+
+    anchor.click();
+    await container.get(IPlatform).taskQueue.yield();
+
+    assert.html.textContent(host, 'c1 params: 42 query: foo=bar fragment:');
+    const path = (container.get(ILocation) as unknown as MockBrowserHistoryLocation).path;
+    assert.match(path, /#\/c1\/42\?foo=bar$/);
+
+    // assert the different parts of the url
+    const url = new URL(path);
+    assert.match(url.pathname, /\/$/);
+    assert.strictEqual(url.search, '');
+    assert.strictEqual(url.hash, '#/c1/42?foo=bar');
+  });
+
+  it('querystring, added to the fragment, can be parsed correctly, when hash-based routing is used', async function () {
+    @customElement({ name: 'c-1', template: `c1 params: \${id} query: \${query} fragment: \${fragment}` })
+    class C1 implements IRouteViewModel {
+
+      private id: string;
+      private query: string;
+      private fragment: string;
+
+      public loading(params: Params, node: RouteNode): void | Promise<void> {
+        this.id = params.id;
+        this.query = node.queryParams.toString();
+        this.fragment = node.fragment;
+      }
+    }
+
+    @route({ routes: [{ id: 'c1', path: 'c1/:id', component: C1 }] })
+    @customElement({ name: 'app', template: '<a href="#/c1/42?foo=bar"></a><au-viewport></au-viewport>' })
+    class App { }
+
+    const { host, container } = await start({ appRoot: App, useHash: true });
+
+    host.querySelector('a').click();
+    await container.get(IPlatform).taskQueue.yield();
+
+    assert.html.textContent(host, 'c1 params: 42 query: foo=bar fragment:');
+    const path = (container.get(ILocation) as unknown as MockBrowserHistoryLocation).path;
+    assert.match(path, /#\/c1\/42\?foo=bar$/);
+
+    // assert the different parts of the url
+    const url = new URL(path);
+    assert.match(url.pathname, /\/$/);
+    assert.strictEqual(url.search, '');
+    assert.strictEqual(url.hash, '#/c1/42?foo=bar');
+  });
+
+  it('querystring, added to the fragment, can be parsed correctly, when hash-based routing is used - with fragment (nested fragment JFF)', async function () {
+    @customElement({ name: 'c-1', template: `c1 params: \${id} query: \${query} fragment: \${fragment}` })
+    class C1 implements IRouteViewModel {
+
+      private id: string;
+      private query: string;
+      private fragment: string;
+
+      public loading(params: Params, node: RouteNode): void | Promise<void> {
+        this.id = params.id;
+        this.query = node.queryParams.toString();
+        this.fragment = node.fragment;
+      }
+    }
+
+    @route({ routes: [{ id: 'c1', path: 'c1/:id', component: C1 }] })
+    @customElement({ name: 'app', template: '<a href="#/c1/42?foo=bar#for-whatever-reason"></a><au-viewport></au-viewport>' })
+    class App { }
+
+    const { host, container } = await start({ appRoot: App, useHash: true });
+
+    host.querySelector('a').click();
+    await container.get(IPlatform).taskQueue.yield();
+
+    assert.html.textContent(host, 'c1 params: 42 query: foo=bar fragment: for-whatever-reason');
+    const path = (container.get(ILocation) as unknown as MockBrowserHistoryLocation).path;
+    assert.match(path, /#\/c1\/42\?foo=bar#for-whatever-reason$/);
+
+    // assert the different parts of the url
+    const url = new URL(path);
+    assert.match(url.pathname, /\/$/);
+    assert.strictEqual(url.search, '');
+    assert.strictEqual(url.hash, '#/c1/42?foo=bar#for-whatever-reason');
+  });
+
+  it('fragment is added to fragment (nested fragment JFF) when using hash-based routing', async function () {
+    @customElement({ name: 'c-1', template: `c1 params: \${id} query: \${query} fragment: \${fragment}` })
+    class C1 implements IRouteViewModel {
+
+      private id: string;
+      private query: string;
+      private fragment: string;
+
+      public loading(params: Params, node: RouteNode): void | Promise<void> {
+        this.id = params.id;
+        this.query = node.queryParams.toString();
+        this.fragment = node.fragment;
+      }
+    }
+
+    @route({ routes: [{ id: 'c1', path: 'c1/:id', component: C1 }] })
+    @customElement({ name: 'app', template: '<au-viewport></au-viewport>' })
+    class App { }
+
+    const { host, container } = await start({ appRoot: App, useHash: true });
+
+    await container.get(IRouter)
+      .load(
+        { component: 'c1', params: { id: '42' } },
+        { queryParams: { foo: 'bar' }, fragment: 'for-whatever-reason' }
+      );
+    assert.html.textContent(host, 'c1 params: 42 query: foo=bar fragment: for-whatever-reason');
+    const path = (container.get(ILocation) as unknown as MockBrowserHistoryLocation).path;
+    assert.match(path, /#\/c1\/42\?foo=bar#for-whatever-reason$/);
+
+    // assert the different parts of the url
+    const url = new URL(path);
+    assert.match(url.pathname, /\/$/);
+    assert.strictEqual(url.search, '');
+    assert.strictEqual(url.hash, '#/c1/42?foo=bar#for-whatever-reason');
+  });
+
   // TODO(sayan): add more tests for title involving children and sibling routes
 
   it('root/child/grandchild/great-grandchild', async function () {
@@ -3695,7 +3841,7 @@ describe('router-lite/smoke-tests.spec.ts', function () {
   });
   // #endregion
 
-  // TODO(sayan): add tests here for the location URL building in relation for sibling, parent/children relationship and viewport name
+  // TODO(sayan): add tests here for the location URL building in relation for viewport name
 
   describe('navigation model', function () {
 

diff --git a/packages/router-lite/src/index.ts b/packages/router-lite/src/index.ts
@@ -111,3 +111,9 @@ export {
 export {
   ViewportAgent,
 } from './viewport-agent';
+
+export {
+  IUrlParser,
+  pathUrlParser,
+  fragmentUrlParser,
+} from './url-parser';
diff --git a/packages/router-lite/src/instructions.ts b/packages/router-lite/src/instructions.ts
@@ -24,6 +24,7 @@ import { RouteExpression } from './route-expression';
 import { mergeURLSearchParams, tryStringify } from './util';
 import { Events, getMessage } from './events';
 import { State } from './viewport-agent';
+import { IUrlParser } from './url-parser';
 
 export const defaultViewportName = 'default';
 export type RouteContextLike = IRouteContext | ICustomElementViewModel | ICustomElementController | HTMLElement;
@@ -34,7 +35,7 @@ export type RouteContextLike = IRouteContext | ICustomElementViewModel | ICustom
  * - `IViewportInstruction`: a viewport instruction object.
  * - `RouteableComponent`: see `RouteableComponent`.
  *
- * NOTE: differs from `Routeable` only in having `IViewportIntruction` instead of `IChildRouteConfig`
+ * NOTE: differs from `Routeable` only in having `IViewportInstruction` instead of `IChildRouteConfig`
  * (which in turn are quite similar, but do have a few minor but important differences that make them non-interchangeable)
  */
 export type NavigationInstruction = string | IViewportInstruction | RouteableComponent;
@@ -290,7 +291,7 @@ export class ViewportInstructionTree {
     }
 
     if (typeof instructionOrInstructions === 'string') {
-      const expr = RouteExpression.parse(instructionOrInstructions, routerOptions.useUrlFragmentHash);
+      const expr = RouteExpression.parse(routerOptions._urlParser.parse(instructionOrInstructions));
       return expr.toInstructionTree(options as NavigationOptions);
     }
 
@@ -334,9 +335,7 @@ export class ViewportInstructionTree {
     return true;
   }
 
-  public toUrl(isFinalInstruction: boolean, useUrlFragmentHash: boolean): string {
-    let pathname: string;
-    let hash: string;
+  public toUrl(isFinalInstruction: boolean, parser: IUrlParser): string {
     let parentPath = '';
 
     if (!isFinalInstruction) {
@@ -359,19 +358,11 @@ export class ViewportInstructionTree {
     }
 
     const currentPath = this.toPath();
-    if (useUrlFragmentHash) {
-      pathname = '/';
-      hash = parentPath.length > 0 ? `#/${parentPath}/${currentPath}` : `#/${currentPath}`;
-    } else {
-      pathname = parentPath.length > 0 ? `${parentPath}/${currentPath}` : currentPath;
-      const fragment = this.fragment;
-      hash = fragment !== null && fragment.length > 0 ? `#${fragment}` : '';
-    }
-
-    let search = this.queryParams.toString();
-    search = search === '' ? '' : `?${search}`;
-
-    return `${pathname}${search}${hash}`;
+    return parser.stringify(
+      parentPath.length > 0 ? `${parentPath}/${currentPath}` : currentPath,
+      this.queryParams,
+      this.fragment
+    );
   }
 
   public toPath(): string {

diff --git a/packages/router-lite/src/options.ts b/packages/router-lite/src/options.ts
@@ -3,6 +3,7 @@ import type { IViewportInstruction, Params, RouteContextLike, RouteableComponent
 import type { RouteNode } from './route-tree';
 import type { Transition } from './router';
 import type { IRouteContext } from './route-context';
+import { IUrlParser, fragmentUrlParser, pathUrlParser } from './url-parser';
 
 export type HistoryStrategy = 'none' | 'replace' | 'push';
 export type ValueOrFunc<T extends string> = T | ((instructions: ViewportInstructionTree) => T);
@@ -16,6 +17,9 @@ function valueOrFuncToValue<T extends string>(instructions: ViewportInstructionT
 export const IRouterOptions = /*@__PURE__*/DI.createInterface<Readonly<RouterOptions>>('RouterOptions');
 export interface IRouterOptions extends Partial<RouterOptions> {}
 export class RouterOptions {
+  /** @internal */
+  public readonly _urlParser: IUrlParser;
+
   protected constructor(
     public readonly useUrlFragmentHash: boolean,
     public readonly useHref: boolean,
@@ -53,7 +57,9 @@ export class RouterOptions {
      * The default value is `true`.
      */
     public readonly restorePreviousRouteTreeOnError: boolean,
-  ) { }
+  ) {
+    this._urlParser = useUrlFragmentHash ? fragmentUrlParser : pathUrlParser;
+   }
 
   public static create(input: IRouterOptions): RouterOptions {
     return new RouterOptions(
@@ -153,7 +159,7 @@ export type FallbackFunction = (viewportInstruction: IViewportInstruction, route
  * - `IChildRouteConfig`: a standalone child route config object.
  * - `RouteableComponent`: see `RouteableComponent`.
  *
- * NOTE: differs from `NavigationInstruction` only in having `IChildRouteConfig` instead of `IViewportIntruction`
+ * NOTE: differs from `NavigationInstruction` only in having `IChildRouteConfig` instead of `IViewportInstruction`
  * (which in turn are quite similar, but do have a few minor but important differences that make them non-interchangeable)
  * as well as `IRedirectRouteConfig`
  */

diff --git a/packages/router-lite/src/resources/load.ts b/packages/router-lite/src/resources/load.ts
@@ -86,7 +86,7 @@ export class LoadCustomAttribute implements ICustomAttributeViewModel {
 
   public valueChanged(): void {
     const router = this._router;
-    const useHash = router.options.useUrlFragmentHash;
+    const options = router.options;
     const component = this.route as NavigationInstruction;
     // this allows binding context to null for navigation from root; unbound vs explicit null binding
     let ctx = this.context;
@@ -102,7 +102,7 @@ export class LoadCustomAttribute implements ICustomAttributeViewModel {
           ? { component, params }
           : component,
         { context: ctx });
-      this._href = instructions.toUrl(false, useHash);
+      this._href = instructions.toUrl(false, options._urlParser);
     } else {
       this._instructions = null;
       this._href = null;
@@ -115,7 +115,7 @@ export class LoadCustomAttribute implements ICustomAttributeViewModel {
       if (this._href === null) {
         this._el.removeAttribute(this.attribute);
       } else {
-        const value = useHash ? this._href : this._locationMgr.addBaseHref(this._href);
+        const value = options.useUrlFragmentHash ? this._href : this._locationMgr.addBaseHref(this._href);
         this._el.setAttribute(this.attribute, value);
       }
     }