unit testing with vitest

[zewa666]

Describe the bug

hey there @ghiscoding

so I'm trying to get my unit tests up and running but noticed a few gotchas. one of them is that vitest loads slickgrid-common in combo with angular-slickgrid as cjs instead of esm. I'll try to understand where that comes from and fix it.

but while at it I noticed that the Sortablejs import now is wrong as the library tries to check the sortablejs.default instead of sortablejs variable. I guess this is due to non-named imports happening in slickgrid.ts

furthermore there is at least one more new place where jsdom causes issues, but I got a workaround for that which I'll add next week.

I'll also create a sample repo as this might involve a couple of more cases

Reproduction

create unit tests for a grid using vite+vitest

Which Framework are you using?

Angular

Environment Info

| Executable          | Version |
| ------------------- | ------- |
| (framework used)    | VERSION |
| Slickgrid-Universal | VERSION |
| TypeScript          | VERSION |
| Browser(s)          | VERSION |
| System OS           | VERSION |

Validations

• Follow our Code of Conduct
• Read the Wikis.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[zewa666]

here is a related ticket for the cjs/esm loading vitest-dev/vitest#4233

[ghiscoding]

hmm on the long run I want to keep ESM only but I think that I should wait another year or two before dropping CJS completely (not everyone uses ESM yet) and that would require to switch from Jest to Vitest for the huge amount of tests that I have which might be long.

Anyway, you can maybe try to play with the package exports of some of the Slickgrid-Universal packages, I was never 100% sure if it works correctly or not. So considering that he wrote that Vitest uses the same as Node then maybe try this change (I don't think I need this node export anyway)

"main": "./dist/cjs/index.js",
  "module": "./dist/esm/index.js",
  "types": "./dist/types/index.d.ts",
  "exports": {
    ".": {
      "types": "./dist/types/index.d.ts",
-     "node": "./dist/cjs/index.js",
      "import": "./dist/esm/index.js",
      "require": "./dist/cjs/index.js"
    },
    "./package.json": "./package.json"
  },

if that doesn't work then maybe try to add "default": "./dist/esm/index.js" as the last exports. I wish that better tools exist to check for hybrid (CJS/ESM) support, all I could really find was Are the Types wrong? and it's telling me that it's all good... anyway I think that if Vitest is able to read "node" exports then that is why it's detected as CJS, I think the ESM should be first (before any CJS export).

but while at it I noticed that the Sortablejs import now is wrong as the library tries to check the sortablejs.default instead of sortablejs variable. I guess this is due to non-named imports happening in slickgrid.ts

why would it do that? it's working fine in Jest and when using the App, so...? I also have allowSyntheticDefaultImports in the tsconfig.base.json, which is supposed to be enough

[zewa666]

so I managed to force vitest to load up the esm instead of cjs version with the following config:

/// <reference types="vitest" />
import { defineConfig } from "vite";
import path from "path";

import angular from "@analogjs/vite-plugin-angular";

// HELPER TO BUILD PATHS TO ESM VERSION
function getAliasedPath(alias: string) {
  return path.resolve(
    __dirname,
    `../node_modules/@slickgrid-universal/${alias}/dist/esm/index.js`
  );
}

export default defineConfig(({ mode }) => ({
  plugins: [angular()],
  test: {
    globals: true,
    setupFiles: ["./src/test-setup.ts"],
    environment: "jsdom",
    include: ["src/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}"],
    reporters: ["default", "junit"],
    coverage: {
      provider: "v8",
      reporter: ["cobertura", "text", "html"],
      reportsDirectory: "./reports"
    },
    outputFile: {
      junit: "./reports/junit.xml"
    },
    css: true,
    // REMOUNT CJS TO ESM REQUESTS
    alias: {
      "@slickgrid-universal/common": getAliasedPath("common"),
      "@slickgrid-universal/row-detail-view-plugin": getAliasedPath(
        "row-detail-view-plugin"
      ),
      "@slickgrid-universal/empty-warning-component": getAliasedPath(
        "empty-warning-component"
      ),
      "@slickgrid-universal/custom-footer-component": getAliasedPath(
        "custom-footer-component"
      ),
      "@slickgrid-universal/pagination-component": getAliasedPath(
        "pagination-component"
      ),
      "@slickgrid-universal/custom-tooltip-plugin": getAliasedPath(
        "custom-tooltip-plugin"
      ),
      "@slickgrid-universal/event-pub-sub": getAliasedPath("event-pub-sub"),
      "@slickgrid-universal/excel-export": getAliasedPath("excel-export"),
      "@slickgrid-universal/odata": getAliasedPath("odata")
    },
    server: {
      deps: {
        inline: ["angular-slickgrid"]
        // THIS ONE IS ALSO NECESSARY TO GET THINGS GOING: https://vitest.dev/config/#server-deps-inline
      }
    }
  },
  define: {
    "import.meta.vitest": mode !== "production"
  }
}));

The other issue that I mentioned was here https://github.com/ghiscoding/slickgrid-universal/blob/master/packages/common/src/core/slickGrid.ts#L2796

where rule.left and rule.right are undefined due to the special treatment of cssRules which isn't compatible with JSDOM. So adding an if clause there fixes my issue:

if (rule.left) {
  rule.left.style.left = `${x}px`;
}
if (rule.right) {
  rule.right.style.right = (((this._options.frozenColumn !== -1 && i > this._options.frozenColumn!) ? this.canvasWidthR : this.canvasWidthL) - x - w) + 'px';
}

[zewa666]

but while at it I noticed that the Sortablejs import now is wrong as the library tries to check the sortablejs.default instead of sortablejs variable. I guess this is due to non-named imports happening in slickgrid.ts

why would it do that? it's working fine in Jest and when using the App, so...? I also have allowSyntheticDefaultImports in the tsconfig.base.json, which is supposed to be enough

I think the issue might be due to a special treatment of those by Vitest itself:
https://vitest.dev/config/#deps-interopdefault

Anyways, I don't wont to care about those anyways since ESM is the way to go and the alias remaps as above work well enough for me without having to change anything on the slickgrid-universal side.

I guess documenting this in our test section should be good enough

[ghiscoding]

instead of the big change you've done in your Vitest config, you should still try the suggestion I proposed above. I have a good feeling that it will help since that "node" was exporting CJS first and you know orders matter in this case so that could explain the issue

[zewa666]

I tried messing around with the exports section and it didn't help. I guess the main reason is as stated in said conversation that the config of package.jsons is only inspected for top-level dependencies, so angular-slickgrid in this case, but not nested deps.

Anyways I've added a PR for the JSDOM fix as well as Doc updates for the Angular Repository

[ghiscoding]

another thing that I was never sure if we are supposed to put the main and module before or after the exports. I guess what would really help is some tools to find exactly the path that it takes to resolve. I think TSC has something for that, but Vitest most probably doesn't

[zewa666]

it would be really strange if the order of properties in a json obj matter as those anyways arent guaranteed. I that can be safely ignored

[ghiscoding]

even if you're surprised, I'm pretty sure that it's still a fact (at least for the exports where I know it totally matters), anyway I've opened PR #1603 with these changes, if it helps then great but if not, well I've tried 🙄

I didn't know it was possible to use Vitest in Angular, it's cool to know, I've never tried AnalogJS

[zewa666]

js world never fails to surprise 🤪

up until recently you needed the full analogjs, but with a recent update now one can pick only the parts necessary for configuring vitest along with an exposed vite config.

[ghiscoding]

@zewa666 I've pushed a new release and that not only include your changes but also the changes I've done to cleanup the exports and other props in PR #1603

One last and very important note, the latest version of Angular 18.1.x is giving problem caused by an indirect dependency string-width that was rewritten as ESM only a while ago but is now being imported by Angular 18.1.x causing CJS error thrown (same as this Yarn GitHub issue)

Unknown error: Error [ERR_REQUIRE_ESM]: require() of ES Module C:\GitHub\Angular-Slickgrid\node_modules\string-width\index.js from C:\GitHub\Angular-Slickgrid\node_modules\cliui\build\index.cjs not supported.
Instead change the require of index.js in C:\GitHub\Angular-Slickgrid\node_modules\cliui\build\index.cjs to a
dynamic import() which is available in all CommonJS modules

I only have that problem in my Angular port, Aurelia & React are unaffected and again it's only after updating to 18.1.x. I also searched in the Angular GitHub project but couldn't find anything recent. I'm wondering if you're also having this problem? ... but anyway at the end of the day, my projects are not the cause of it and personally I don't like project that went for ESM only when an hybrid approach is a lot better and what I follow (support both CJS and prefer ESM)

temp patch

a temp patch to keep updating to latest Angular versions is to use something like Yarn
"resolutions": { "string-width": "4.2.3" } or anything similar to override deps

EDIT

as per Yarn issue this might a Yarn legacy issue only, so anyway I applied the patch in Angular-Slickgrid PR

[zewa666]

havent yet updated to the latest v18 version. but I wonder whether it will affect vite and vitest projects as there shouldnt be any cjs involved. thanks for the heads up and for the new release