ghiscoding / slickgrid-universal

Slickgrid-Universal is a monorepo which includes all Editors, Filters, Extensions, Services related to SlickGrid usage and is also Framework Agnostic
https://ghiscoding.github.io/slickgrid-universal/
Other
90 stars 29 forks source link

perf: use SortableJS ESM import to save 1kb #1621

Closed ghiscoding closed 4 months ago

ghiscoding commented 4 months ago

I'm not sure if it's worth it or not but we seem to be using 1kb from the zip when using the ESM build of SortableJS. However the thing that is not as great is that @types/sortablejs is not recognized when using import Sortable from 'sortablejs/modular/sortable.core.esm.js';

So I had to use something like below with a @ts-ignore on the ESM import

// @ts-ignore: use the ESM imports for smaller build, it however doesn't play nice with @types/sortablejs
import Sortable from 'sortablejs/modular/sortable.core.esm.js';
import type { Options as SortableOptions, SortableEvent } from 'sortablejs'; // from @types/sortablejs

before

image

after

image

stackblitz[bot] commented 4 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 14.7%. Comparing base (e34971d) to head (986fdb0). Report is 21 commits behind head on master.

Files Patch % Lines
...es/common/src/extensions/slickDraggableGrouping.ts 0.0% 2 Missing :warning:
packages/common/src/core/slickGrid.ts 0.0% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1621 +/- ## ========================================= - Coverage 99.8% 14.7% -85.0% ========================================= Files 198 198 Lines 21795 21796 +1 Branches 7302 7161 -141 ========================================= - Hits 21734 3202 -18532 - Misses 61 16874 +16813 - Partials 0 1720 +1720 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ghiscoding commented 4 months ago

hmm that breaks Jest unit tests since Jest only supports CJS, perhaps I should revisit this if (when) I move from Jest to Vitest

zewa666 commented 4 months ago

well you could potentially make Jest play nice with ESM. https://jestjs.io/docs/ecmascript-modules I tried that though quite a while ago and it was a mess. Hence why I moved to vitest

ghiscoding commented 4 months ago

well you could potentially make Jest play nice with ESM. https://jestjs.io/docs/ecmascript-modules I tried that though quite a while ago and it was a mess. Hence why I moved to vitest

I tried doing that approach in Lerna-Lite and it was also a big mess, so I decided to switch to Vitest on Lerna-Lite and was happy with the change. The only downside is that in Lerna-Lite, running Vitest is really slow on Windows when using a lot of child_process that Lerna uses a lot, however that won't be the case here, so I do want to migrate the project to Vitest eventually but considering that I have close to 5k unit tests, that for sure be a challenge. Anyway, that is super low priority.

zewa666 commented 4 months ago

glad to hear its not me being the only one having trouble with getting esm play nice with jest. 😅

ghiscoding commented 4 months ago

closing for now, might revisit if and when we migrate to Vitest to support ESM