refactor

packages/remark-lsx/src/components/Lsx.tsx

@@ -1,7 +1,7 @@
 import React, { useCallback, useMemo } from 'react';
 
 
-import { useSWRxLsx } from '../stores/lsx';
+import { useSWRxLsx } from '../stores/lsx/lsx';
 import { generatePageNodeTree } from '../utils/page-node';
 
 import { LsxListView } from './LsxPageList/LsxListView';
@@ -38,7 +38,7 @@ const LsxSubstance = React.memo(({
     return new LsxContext(prefix, options);
   }, [depth, filter, num, prefix, reverse, sort, except]);
 
-  const { data, error, isLoading } = useSWRxLsx(lsxContext, isImmutable);
+  const { data, error, isLoading } = useSWRxLsx(lsxContext.pagePath, lsxContext.options, isImmutable);
 
   const hasError = error != null;
   const errorMessage = error?.message;

packages/remark-lsx/src/interfaces/api.ts

@@ -0,0 +1,14 @@
+export type LsxApiOptions = {
+  depth?: string,
+  filter?: string,
+  except?: string,
+  sort?: string,
+  reverse?: string,
+}
+
+export type LsxApiParams = {
+  pagePath: string,
+  offset?: number,
+  limit?: number,
+  options?: LsxApiOptions,
+}

packages/remark-lsx/src/server/routes/list-pages/add-num-condition.spec.ts

@@ -1,92 +1,80 @@
-import { OptionParser } from '@growi/core/dist/plugin';
 import createError from 'http-errors';
 import { mock } from 'vitest-mock-extended';
 
 import { addNumCondition } from './add-num-condition';
 import type { PageQuery } from './generate-base-query';
 
-describe('addNumCondition()', () => {
+describe('addNumCondition() throws 400 http-errors instance ', () => {
 
-  const queryMock = mock<PageQuery>();
-
-  it('set limit with the specified number', () => {
-    // setup
-    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
-
-    const queryLimitResultMock = mock<PageQuery>();
-    queryMock.limit.calledWith(99).mockImplementation(() => queryLimitResultMock);
-
-    // when
-    const result = addNumCondition(queryMock, 99);
-
-    // then
-    expect(queryMock.limit).toHaveBeenCalledWith(99);
-    expect(result).toEqual(queryLimitResultMock);
-    expect(parseRangeSpy).not.toHaveBeenCalled();
-  });
-
-  it('returns the specified qeury as-is when the option value is invalid', () => {
-    // setup
-    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
-
-    // when
-    const result = addNumCondition(queryMock, 'invalid string');
-
-    // then
-    expect(queryMock.limit).not.toHaveBeenCalled();
-    expect(parseRangeSpy).toHaveBeenCalledWith('invalid string');
-    expect(result).toEqual(queryMock);
-  });
+  it.concurrent.each`
+    offset  | limit
+    ${-1}   | ${9}
+    ${1}    | ${-9}
+  `('when the specified condition is { offset: $offset, limit: $limit }', ({ offset, limit }) => {
 
-  it('throws 400 http-errors instance when the start value is smaller than 1', () => {
     // setup
-    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
+    const queryMock = mock<PageQuery>();
 
     // when
-    const caller = () => addNumCondition(queryMock, '-1:10');
+    const caller = () => addNumCondition(queryMock, offset, limit);
 
     // then
-    expect(caller).toThrowError(createError(400, 'specified num is [-1:10] : the start must be larger or equal than 1'));
+    expect(caller).toThrowError(createError(400, "Both specified 'offset' or 'limit must be larger or equal than 0"));
+    expect(queryMock.skip).not.toHaveBeenCalledWith();
     expect(queryMock.limit).not.toHaveBeenCalledWith();
-    expect(parseRangeSpy).toHaveBeenCalledWith('-1:10');
   });
-
 });
 
 
-describe('addNumCondition() set skip and limit with the range string', () => {
+describe('addNumCondition() set skip and limit with', () => {
 
   it.concurrent.each`
-    optionsNum    | expectedSkip    | expectedLimit   | isExpectedToSetLimit
-    ${'1:10'}     | ${0}            | ${10}           | ${true}
-    ${'3:'}       | ${2}            | ${-1}           | ${false}
-  `("'$optionsNum", ({
-    optionsNum, expectedSkip, expectedLimit, isExpectedToSetLimit,
+    offset  | limit                   | expectedSkip   | expectedLimit
+    ${1}    | ${Number.MAX_VALUE}     | ${1}           | ${null}
+    ${0}    | ${10}                   | ${null}        | ${10}
+    ${0}    | ${Number.MAX_VALUE}     | ${null}        | ${null}
+    ${NaN}  | ${NaN}                  | ${null}        | ${null}
+  `("{ offset: $offset, limit: $limit }'", ({
+    offset, limit, expectedSkip, expectedLimit,
   }) => {
     // setup
     const queryMock = mock<PageQuery>();
 
+    // result for q.skip()
     const querySkipResultMock = mock<PageQuery>();
     queryMock.skip.calledWith(expectedSkip).mockImplementation(() => querySkipResultMock);
-
+    // result for q.limit()
     const queryLimitResultMock = mock<PageQuery>();
-    querySkipResultMock.limit.calledWith(expectedLimit).mockImplementation(() => queryLimitResultMock);
-
-    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
+    queryMock.limit.calledWith(expectedLimit).mockImplementation(() => queryLimitResultMock);
+    // result for q.skil().limit()
+    const querySkipAndLimitResultMock = mock<PageQuery>();
+    querySkipResultMock.limit.calledWith(expectedLimit).mockImplementation(() => querySkipAndLimitResultMock);
 
     // when
-    const result = addNumCondition(queryMock, optionsNum);
+    const result = addNumCondition(queryMock, offset, limit);
 
     // then
-    expect(parseRangeSpy).toHaveBeenCalledWith(optionsNum);
-    expect(queryMock.skip).toHaveBeenCalledWith(expectedSkip);
-    if (isExpectedToSetLimit) {
-      expect(querySkipResultMock.limit).toHaveBeenCalledWith(expectedLimit);
-      expect(result).toEqual(queryLimitResultMock);
+    if (expectedSkip != null) {
+      expect(queryMock.skip).toHaveBeenCalledWith(expectedSkip);
+      if (expectedLimit != null) {
+        expect(querySkipResultMock.limit).toHaveBeenCalledWith(expectedLimit);
+        expect(result).toEqual(querySkipAndLimitResultMock); // q.skip().limit()
+      }
+      else {
+        expect(querySkipResultMock.limit).not.toHaveBeenCalled();
+        expect(result).toEqual(querySkipResultMock); // q.skil()
+      }
     }
     else {
-      expect(querySkipResultMock.limit).not.toHaveBeenCalled();
-      expect(result).toEqual(querySkipResultMock);
+      expect(queryMock.skip).not.toHaveBeenCalled();
+      if (expectedLimit != null) {
+        expect(queryMock.limit).toHaveBeenCalledWith(expectedLimit);
+        expect(result).toEqual(queryLimitResultMock); // q.limit()
+      }
+      else {
+        expect(queryMock.limit).not.toHaveBeenCalled();
+        expect(result).toEqual(queryMock); // as-is
+      }
     }
   });
 

packages/remark-lsx/src/server/routes/list-pages/add-num-condition.ts

@@ -1,38 +1,27 @@
-import { OptionParser } from '@growi/core/dist/plugin';
 import createError from 'http-errors';
 
 import type { PageQuery } from './generate-base-query';
 
 
+const DEFAULT_PAGES_NUM = 50;
+
 /**
  * add num condition that limit fetched pages
  */
-export const addNumCondition = (query: PageQuery, optionsNum: string | number): PageQuery => {
+export const addNumCondition = (query: PageQuery, offset = 0, limit = DEFAULT_PAGES_NUM): PageQuery => {
 
-  if (typeof optionsNum === 'number') {
-    return query.limit(optionsNum);
+  // check offset
+  if (offset < 0 || limit < 0) {
+    throw createError(400, "Both specified 'offset' or 'limit must be larger or equal than 0");
   }
 
-  const range = OptionParser.parseRange(optionsNum);
-
-  if (range == null) {
-    return query;
+  let q = query;
+  if (offset !== 0) {
+    q = q.skip(offset);
   }
-
-  const start = range.start;
-  const end = range.end;
-
-  // check start
-  if (start < 1) {
-    throw createError(400, `specified num is [${start}:${end}] : the start must be larger or equal than 1`);
-  }
-
-  const skip = start - 1;
-  const limit = end - skip;
-
-  if (limit < 0) {
-    return query.skip(skip);
+  if (limit !== Number.MAX_VALUE) {
+    q = q.limit(limit);
   }
 
-  return query.skip(skip).limit(limit);
+  return q;
 };

packages/remark-lsx/src/server/routes/list-pages/index.spec.ts

@@ -44,19 +44,33 @@ describe('listPages', () => {
 
   describe('with num option', () => {
 
-    mocks.generateBaseQueryMock.mockImplementation(() => vi.fn());
-    mocks.getToppageViewersCountMock.mockImplementation(() => 99);
+    beforeAll(() => {
+      mocks.generateBaseQueryMock.mockImplementation(() => vi.fn());
+      mocks.getToppageViewersCountMock.mockImplementation(() => 99);
+    });
 
     it('returns 200 HTTP response', async() => {
       // setup
       const reqMock = mock<Request & { user: IUser }>();
       reqMock.query = { pagePath: '/Sandbox' };
 
-      const pageMock = mock<IPage>();
       const queryMock = mock<PageQuery>();
+
+      // setup addNumCondition
+      mocks.addNumConditionMock.mockImplementation(() => queryMock);
+      // setup addSortCondition
+      mocks.addSortConditionMock.mockImplementation(() => queryMock);
+
+      // setup query.exec()
+      const pageMock = mock<IPage>();
       queryMock.exec.mockImplementation(async() => [pageMock]);
       mocks.addSortConditionMock.mockImplementation(() => queryMock);
 
+      // setup query.clone().count()
+      const queryClonedMock = mock<PageQuery>();
+      queryMock.clone.mockImplementationOnce(() => queryClonedMock);
+      queryClonedMock.count.mockResolvedValue(9);
+
       const resMock = mock<Response>();
       const resStatusMock = mock<Response>();
       resMock.status.calledWith(200).mockReturnValue(resStatusMock);
@@ -72,6 +86,7 @@ describe('listPages', () => {
       expect(resMock.status).toHaveBeenCalledOnce();
       expect(resStatusMock.send).toHaveBeenCalledWith({
         pages: [pageMock],
+        total: 9,
         toppageViewersCount: 99,
       });
     });

packages/remark-lsx/src/server/routes/list-pages/index.ts

@@ -5,6 +5,8 @@ import escapeStringRegexp from 'escape-string-regexp';
 import type { Request, Response } from 'express';
 import createError, { isHttpError } from 'http-errors';
 
+import { LsxApiParams } from '../../../interfaces/api';
+
 import { addDepthCondition } from './add-depth-condition';
 import { addNumCondition } from './add-num-condition';
 import { addSortCondition } from './add-sort-condition';
@@ -12,9 +14,6 @@ import { generateBaseQuery, type PageQuery } from './generate-base-query';
 import { getToppageViewersCount } from './get-toppage-viewers-count';
 
 
-const DEFAULT_PAGES_NUM = 50;
-
-
 const { addTrailingSlash } = pathUtils;
 
 /**
@@ -57,37 +56,25 @@ function addExceptCondition(query, pagePath, optionsFilter): PageQuery {
 }
 
 
-export type ListPagesOptions = {
-  depth?: string,
-  num?: string,
-  filter?: string,
-  except?: string,
-  sort?: string,
-  reverse?: string,
-}
-
 export const listPages = async(req: Request & { user: IUser }, res: Response): Promise<Response> => {
   const user = req.user;
 
-  let pagePath: string;
-  let options: ListPagesOptions | undefined;
-
-  try {
-    // TODO: use express-validator
-    if (req.query.pagePath == null) {
-      throw new Error("The 'pagePath' query must not be null.");
-    }
-
-    pagePath = req.query.pagePath?.toString();
-    if (req.query.options != null) {
-      options = JSON.parse(req.query.options.toString());
-    }
-  }
-  catch (error) {
-    return res.status(400).send(error);
+  // TODO: use express-validator
+  if (req.query.pagePath == null) {
+    return res.status(400).send("The 'pagePath' query must not be null.");
   }
 
-  const builder = await generateBaseQuery(pagePath, user);
+  const params: LsxApiParams = {
+    pagePath: req.query.pagePath.toString(),
+    offset: req.query?.offset != null ? Number(req.query.offset) : undefined,
+    limit: req.query?.limit != null ? Number(req.query?.limit) : undefined,
+    options: req.query?.options != null ? JSON.parse(req.query.options.toString()) : {},
+  };
+
+  const {
+    pagePath, offset, limit, options,
+  } = params;
+  const builder = await generateBaseQuery(params.pagePath, user);
 
   // count viewers of `/`
   let toppageViewersCount;
@@ -102,7 +89,7 @@ export const listPages = async(req: Request & { user: IUser }, res: Response): P
   try {
     // depth
     if (options?.depth != null) {
-      query = addDepthCondition(query, pagePath, options.depth);
+      query = addDepthCondition(query, params.pagePath, options.depth);
     }
     // filter
     if (options?.filter != null) {
@@ -112,15 +99,13 @@ export const listPages = async(req: Request & { user: IUser }, res: Response): P
       query = addExceptCondition(query, pagePath, options.except);
     }
 
-    const total = await query.clone().count();
-
     // num
-    const optionsNum = options?.num || DEFAULT_PAGES_NUM;
-    query = addNumCondition(query, optionsNum);
+    query = addNumCondition(query, offset, limit);
     // sort
     query = addSortCondition(query, options?.sort, options?.reverse);
 
     const pages = await query.exec();
+    const total = await query.clone().count();
     return res.status(200).send({ pages, total, toppageViewersCount });
   }
   catch (error) {

packages/remark-lsx/src/stores/lsx.tsx → packages/remark-lsx/src/stores/lsx/lsx.tsx

@@ -2,7 +2,9 @@ import type { IPageHasId } from '@growi/core';
 import axios from 'axios';
 import useSWR, { SWRResponse } from 'swr';
 
-import { LsxContext } from '../components/lsx-context';
+import { LsxApiParams } from '../../interfaces/api';
+
+import { parseNumOption } from './parse-num-option';
 
 type LsxResponse = {
   pages: IPageHasId[],
@@ -10,19 +12,22 @@ type LsxResponse = {
   toppageViewersCount: number,
 }
 
-export const useSWRxLsx = (lsxContext: LsxContext, isImmutable?: boolean): SWRResponse<LsxResponse, Error> => {
-  const { pagePath, options } = lsxContext;
-
+export const useSWRxLsx = (pagePath: string, options?: Record<string, string|undefined>, isImmutable?: boolean): SWRResponse<LsxResponse, Error> => {
   return useSWR(
     ['/_api/lsx', pagePath, options, isImmutable],
     async([endpoint, pagePath, options]) => {
       try {
-        const res = await axios.get<LsxResponse>(endpoint, {
-          params: {
-            pagePath,
-            options,
-          },
-        });
+        const offsetAndLimit = options?.num != null
+          ? parseNumOption(options.num)
+          : null;
+
+        const params: LsxApiParams = {
+          pagePath,
+          offset: offsetAndLimit?.offset,
+          limit: offsetAndLimit?.limit,
+          options,
+        };
+        const res = await axios.get<LsxResponse>(endpoint, { params });
         return res.data;
       }
       catch (err) {

packages/remark-lsx/src/stores/lsx/parse-num-option.spec.ts

@@ -0,0 +1,64 @@
+import { OptionParser } from '@growi/core/dist/plugin';
+
+import { parseNumOption } from './parse-num-option';
+
+describe('addNumCondition()', () => {
+
+  it('set limit with the specified number', () => {
+    // setup
+    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
+
+    // when
+    const result = parseNumOption('99');
+
+    // then
+    expect(result).toEqual({ limit: 99 });
+    expect(parseRangeSpy).not.toHaveBeenCalled();
+  });
+
+  it('returns null when the option value is invalid', () => {
+    // setup
+    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
+
+    // when
+    const result = parseNumOption('invalid string');
+
+    // then
+    expect(parseRangeSpy).toHaveBeenCalledWith('invalid string');
+    expect(result).toBeNull();
+  });
+
+  it('throws an error when the start value is smaller than 1', () => {
+    // setup
+    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
+
+    // when
+    const caller = () => parseNumOption('-1:10');
+
+    // then
+    expect(caller).toThrowError();
+    expect(parseRangeSpy).toHaveBeenCalledWith('-1:10');
+  });
+
+});
+
+
+describe('addNumCondition() set skip and limit with the range string', () => {
+
+  it.concurrent.each`
+    optionsNum    | expected
+    ${'1:10'}     | ${{ offset: 0, limit: 10 }}
+    ${'3:'}       | ${{ offset: 2, limit: Number.MAX_VALUE }}
+  `("'$optionsNum", ({ optionsNum, expected }) => {
+    // setup
+    const parseRangeSpy = vi.spyOn(OptionParser, 'parseRange');
+
+    // when
+    const result = parseNumOption(optionsNum);
+
+    // then
+    expect(parseRangeSpy).toHaveBeenCalledWith(optionsNum);
+    expect(result).toEqual(expected);
+  });
+
+});

packages/remark-lsx/src/stores/lsx/parse-num-option.ts

@@ -0,0 +1,35 @@
+import { OptionParser } from '@growi/core/dist/plugin';
+
+export type ParseNumOptionResult = { offset: number, limit?: number } | { offset?: number, limit: number };
+
+/**
+ * add num condition that limit fetched pages
+ */
+export const parseNumOption = (optionsNum: string): ParseNumOptionResult | null => {
+
+  if (Number.isInteger(Number(optionsNum))) {
+    return { limit: Number(optionsNum) };
+  }
+
+  const range = OptionParser.parseRange(optionsNum);
+
+  if (range == null) {
+    return null;
+  }
+
+  const start = range.start;
+  const end = range.end;
+
+  // check start
+  if (start < 1) {
+    throw new Error(`The specified option 'num' is [${start}:${end}] : the start must be larger or equal than 1`);
+  }
+
+  const offset = start - 1;
+  const limit = end - offset;
+
+  return {
+    offset,
+    limit: (limit > 0) ? limit : Number.MAX_VALUE,
+  };
+};