Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static typing extremely slow #651

Open
laugri opened this issue Oct 11, 2024 · 5 comments
Open

Static typing extremely slow #651

laugri opened this issue Oct 11, 2024 · 5 comments
Labels
enhancement New feature or request help wanted Extra attention is needed possible discussion Not exactly a bug or a feature request, but rather something that could or should be discussed

Comments

@laugri
Copy link

laugri commented Oct 11, 2024

Describe the bug

This isn't a bug per se, I might be doing something wrong with the library! When I am working on the file that has the datatable in it in VA Code, static typing with typescript becomes incredibly slow (e.g. I have to wait up to 10s for the typing to update after adding a line of code).

To Reproduce

Here is my file in case there is something obvious I missed.

import { DataTable, DataTableSortStatus } from "mantine-datatable";
import WorkerName from "./TableRow/WorkerName.tsx";
import ChangesPreview from "./TableRow/ChangesPreview.tsx";
import Distance from "./TableRow/Distance/Distance.tsx";
import HourCounter from "./TableRow/HourCounterCell.tsx";
import { sortRecords } from "./utils.ts";
import RowExpansion from "./RowExpansion.tsx";
import RowActions from "./TableRow/Actions/RowActions.tsx";
import { useEffect, useState } from "react";
import { SubstitutionOption } from "#bcp/services/substitution/options/SubstitutionOption.ts";
import { Shift } from "#bcp/services/ximiAPI/scheduling/interventions/Shift.ts";
import { Stack } from "@mantine/core";
import Schedule from "./Schedule/Schedule.tsx";

export default function SubstituteOptionsTable({
  options,
  shiftToReplace,
  parentOption,
  parentShiftToReplace,
}: {
  options: SubstitutionOption[];
  shiftToReplace: Shift;
  parentOption?: SubstitutionOption;
  parentShiftToReplace?: Shift;
}) {
  const [records, setRecords] = useState(sortRecords(options));
  const [sortStatus, setSortStatus] = useState<
    DataTableSortStatus<SubstitutionOption>
  >({
    columnAccessor: "worker",
    direction: "asc",
  });

  useEffect(() => {
    const data = sortRecords(
      records,
      sortStatus.columnAccessor,
      toto
    ) as SubstitutionOption[];
    setRecords(sortStatus.direction === "desc" ? data.reverse() : data);
  }, [sortStatus]);

  function removeRecord(workerId: string) {
    setRecords((prev) =>
      prev.filter((record) => record.worker.id !== workerId),
    );
  }

  return (
    <DataTable
      withTableBorder
      borderRadius="md"
      verticalSpacing="md"
      highlightOnHover
      minHeight={150}
      noRecordsText="Aucune option trouvée"
      records={records}
      idAccessor="id"
      sortStatus={sortStatus}
      onSortStatusChange={setSortStatus}
      columns={[
        {
          accessor: "worker",
          title: "Intervenant",
          width: 216,
          render: (record) => <WorkerName option={record} />,
          sortable: true,
        },
        {
          accessor: "hourChanges",
          title: "Changements",
          noWrap: true,
          render: (record) => <ChangesPreview option={record} />,
        },
        {
          accessor: "route",
          title: "Trajets avant-après",
          render: (record) => <Distance record={record} />,
          sortable: true,
        },
        {
          accessor: "hourCounter",
          title: "Solde d'heures",
          render: (record) => (
            <HourCounter
              workerId={record.worker.id}
              targetDate={shiftToReplace.start}
              contract={record.worker.contract!}
            />
          ),
          sortable: true,
        },
        {
          accessor: "actions",
          title: "",
          render: (record) => (
            <RowActions
              option={record}
              shiftToReplace={shiftToReplace}
              removeRecord={removeRecord}
              parentOption={parentOption}
              parentShiftToReplace={parentShiftToReplace}
            />
          ),
        },
      ]}
      rowExpansion={{
        allowMultiple: true,
        content: ({ record }) =>
          parentOption && parentShiftToReplace ? (
            <Stack p={16} bg="white">
              <Schedule option={record} shiftToReplace={shiftToReplace} />
            </Stack>
          ) : (
            <RowExpansion option={record} shiftToReplace={shiftToReplace} />
          ),
        collapseProps: {
          transitionDuration: 0,
        },
      }}
    />
  );
}

Expected behavior
I expect no performance change compared to the rest of my codebase (typescript-wise).

I am on version 7.12.4 but it was like this before as well.

Thanks !

@ksdme
Copy link

ksdme commented Oct 12, 2024

+1 and I also see tsc build failures with out of memory errors.

> frontend@0.0.0 build
> tsc -b && vite build


<--- Last few GCs --->

[2307328:0x7305c90]    42951 ms: Mark-Compact 4050.3 (4137.4) -> 4038.5 (4141.1) MB, 1225.03 / 0.00 ms  (average mu = 0.617, current mu = 0.011) allocation failure; scavenge might not succeed
[2307328:0x7305c90]    44821 ms: Mark-Compact 4054.5 (4141.1) -> 4042.7 (4145.4) MB, 1851.79 / 0.00 ms  (average mu = 0.357, current mu = 0.010) allocation failure; scavenge might not succeed


<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----

 1: 0xca5580 node::Abort() [node]
 2: 0xb781f9  [node]
 3: 0xeca4d0 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node]
 4: 0xeca7b7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node]
 5: 0x10dc505  [node]
 6: 0x10f4388 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 7: 0x10ca4a1 v8::internal::HeapAllocator::AllocateRawWithLightRetrySlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 8: 0x10cb635 v8::internal::HeapAllocator::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 9: 0x10a8c86 v8::internal::Factory::NewFillerObject(int, v8::internal::AllocationAlignment, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node]
10: 0x1503a16 v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long*, v8::internal::Isolate*) [node]
11: 0x7591dbcd9ef6
Aborted (core dumped)

I have got plenty of memory on my machine (32G) and allocating more to the node process with a flag doesn't do anything either.

@ksdme
Copy link

ksdme commented Oct 12, 2024

Stubbing out the type definitions and replacing them with a simpler version seems to work for me. But, obviously, the downside is that the types are not complete. In any case, if you are interested, here's my temporary solution,

mantine-datatable.d.ts

// https://github.com/icflorescu/mantine-datatable/issues/651
declare module "mantine-datatable" {
  // https://icflorescu.github.io/mantine-datatable/type-definitions/
  export interface DataTableProps<T> {
    columns?: { accessor: keyof T | string, title?: string }[]
    records?: T[]

    totalRecords?: number
    page?: number
    recordsPerPage?: number
    onPageChange?: (page: number) => void

    minHeight?: number
    borderRadius?: "xs" | "sm" | "md" | "lg" | "xl"
    withTableBorder?: boolean
    highlightOnHover?: boolean
  }

  export function DataTable<T>(props: DataTableProps<T>): React.ReactNode;
}

ts-config.json

diff --git a/frontend/tsconfig.app.json b/frontend/tsconfig.app.json
index 1540e7c..03f52ff 100644
--- a/frontend/tsconfig.app.json
+++ b/frontend/tsconfig.app.json
@@ -20,9 +20,15 @@
     "noUnusedParameters": true,
     "noFallthroughCasesInSwitch": true,

+    /* Additional Types */
+    "types": [
+      // https://github.com/icflorescu/mantine-datatable/issues/651
+      "./mantine-datatable"
+    ],
+
     /* Aliases */
     "paths": {
-      "@/*": ["./src/*"]
+      "@/*": ["./src/*"],
     }
   },
   "include": ["src"]

@sonseait
Copy link

sonseait commented Nov 22, 2024

diff --git a/dist/index.d.ts b/dist/index.d.ts
index 9cec316845efa273ff4aaa430cd8b3e8c689d7e4..eaf8816b131e22382d09f14cddbef02b037dbead 100644
--- a/dist/index.d.ts
+++ b/dist/index.d.ts
@@ -239,54 +239,17 @@ type DataTableEmptyStateProps = {
 };
 
 type DataTableOuterBorderProps = {
-    withTableBorder?: never;
-    borderRadius?: never;
-} | {
     /**
      * If true, table will have border.
      */
-    withTableBorder: boolean;
+    withTableBorder?: boolean;
     /**
      * Table border radius.
      */
     borderRadius?: MantineSize | (string & NonNullable<unknown>) | number;
 };
 
-type DataTablePageSizeSelectorProps = {
-    onRecordsPerPageChange?: never;
-    recordsPerPageOptions?: never;
-    recordsPerPageLabel?: never;
-} | {
-    /**
-     * Callback fired a new page size is selected.
-     * Receives new page size as argument.
-     */
-    onRecordsPerPageChange: (recordsPerPage: number) => void;
-    /**
-     * Array of page sizes (numbers) to show in records per page selector.
-     */
-    recordsPerPageOptions: number[];
-    /**
-     * Label for records per page selector.
-     */
-    recordsPerPageLabel?: string;
-};
-
-type DataTablePaginationProps = ({
-    paginationWithEdges?: never;
-    paginationWithControls?: never;
-    page?: never;
-    onPageChange?: never;
-    totalRecords?: never;
-    recordsPerPage?: never;
-    paginationActiveTextColor?: never;
-    paginationActiveBackgroundColor?: never;
-    paginationSize?: never;
-    loadingText?: never;
-    paginationText?: never;
-    paginationWrapBreakpoint?: never;
-    getPaginationControlProps?: never;
-} | {
+type DataTablePaginationProps = {
     /**
      * Whether to show first and last page navigation buttons.
      */
@@ -299,20 +262,20 @@ type DataTablePaginationProps = ({
      * Current page number (1-based).
      * If provided, a pagination component is shown.
      */
-    page: number;
+    page?: number;
     /**
      * Callback fired after page change.
      * Receives the new page number as argument.
      */
-    onPageChange: (page: number) => void;
+    onPageChange?: (page: number) => void;
     /**
      * Total number of records in the dataset.
      */
-    totalRecords: number | undefined;
+    totalRecords?: number | undefined;
     /**
      * Number of records per page.
      */
-    recordsPerPage: number;
+    recordsPerPage?: number;
     /**
      * Pagination component size.
      * @default `sm`
@@ -363,7 +326,20 @@ type DataTablePaginationProps = ({
      * Useful for improving accessibility.
      */
     getPaginationControlProps?: (control: 'first' | 'last' | 'previous' | 'next') => Record<string, unknown>;
-}) & DataTablePageSizeSelectorProps;
+    /**
+     * Callback fired a new page size is selected.
+     * Receives new page size as argument.
+     */
+    onRecordsPerPageChange?: (recordsPerPage: number) => void;
+    /**
+     * Array of page sizes (numbers) to show in records per page selector.
+     */
+    recordsPerPageOptions?: number[];
+    /**
+     * Label for records per page selector.
+     */
+    recordsPerPageLabel?: string;
+};
 
 type DataTableColorProps<T> = {
     /**
@@ -592,16 +568,6 @@ type DataTableScrollProps = {
 type DataTableSelectionTrigger = 'cell' | 'checkbox';
 
 type DataTableSelectionProps<T = Record<string, unknown>> = {
-    selectionTrigger?: never;
-    selectedRecords?: never;
-    onSelectedRecordsChange?: never;
-    isRecordSelectable?: never;
-    selectionCheckboxProps?: never;
-    getRecordSelectionCheckboxProps?: never;
-    allRecordsSelectionCheckboxProps?: never;
-    selectionColumnClassName?: never;
-    selectionColumnStyle?: never;
-} | {
     /**
      * Defines how selection is triggered.
      * @default 'checkbox'
@@ -663,20 +629,16 @@ type DataTableSortStatus<T = Record<string, unknown>> = {
     direction: 'asc' | 'desc';
 };
 
-type DataTableSortProps<T = Record<string, unknown>> = ({
-    sortStatus?: never;
-    onSortStatusChange?: never;
-} | {
+type DataTableSortProps<T = Record<string, unknown>> = {
     /**
      * Current sort status (sort column accessor & direction).
      */
-    sortStatus: DataTableSortStatus<T>;
+    sortStatus?: DataTableSortStatus<T>;
     /**
      * Callback fired after change of sort status.
      * Receives the new sort status as argument.
      */
     onSortStatusChange?: (sortStatus: DataTableSortStatus<T>) => void;
-}) & {
     /**
      * Custom sort icons.
      */

I think the main problem is intersection types, I convert most of them to static types and make all properties optional, It may break some things but this one helps me

@icflorescu
Copy link
Owner

I think the main problem is intersection types

I was wondering, is it the intersection types, or my way of defining the accessors like keyof T | (string & NonNullable<unknown>), in order to enable VSCode to offer key name suggestions?

I'd be grateful if someone could find the time to investigate this a bit...

I'm open to rewrite the accessor definitions if that speeds things up.

@icflorescu icflorescu added enhancement New feature or request help wanted Extra attention is needed possible discussion Not exactly a bug or a feature request, but rather something that could or should be discussed labels Nov 26, 2024
@sonseait
Copy link

sonseait commented Nov 26, 2024

I think the main problem is intersection types

I was wondering, is it the intersection types, or my way of defining the accessors like keyof T | (string & NonNullable<unknown>), in order to enable VSCode to offer key name suggestions?

I'd be grateful if someone could find the time to investigate this a bit...

I'm open to rewrite the accessor definitions if that speeds things up.

I think the accessor type is fine, I tried to comment on some parts of DataTableProps and I realized when I commented on the props that have intersection types then I could get suggestions faster, like this one

image

So I removed the part that has "never" type. I think I understand your idea that you tried to make the suggestions more correctly, but I don't know, maybe that is a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed possible discussion Not exactly a bug or a feature request, but rather something that could or should be discussed
Projects
None yet
Development

No branches or pull requests

4 participants