vasturiano / icicle-chart

A partition / icicle interactive chart web component for visualizing hierarchical data
https://vasturiano.github.io/icicle-chart/example/flare/
MIT License
37 stars 12 forks source link

Migration to ES Modules broke Jest unit testing #16

Open joaosamouco opened 1 year ago

joaosamouco commented 1 year ago

Describe the bug

The most recent change to this and other packages like kapsule, accessor-fn, float-tooltip, and probably some more, broke my unit-tests pipeline that uses Jest. Jest support for ES Modules is still in experimental phase so things are expected to break.

Details:

    /home/user/code/app/node_modules/float-tooltip/dist/float-tooltip.mjs:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { select, pointer } from 'd3-selection';
                                                                                      ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      3 | import React, { useEffect, useRef, useState } from "react";
      4 | import PropTypes from "prop-types";
    > 5 | import Icicle from "icicle-chart";
        | ^
      6 | import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
      7 | import {

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1605:14)
      at Object.<anonymous> (node_modules/icicle-chart/dist/icicle-chart.common.js:12:15)
      at Object.<anonymous> (lib/components/TreeMap/TreeMap.tsx:5:1)
      at Object.<anonymous> (lib/components/TreeMap/TreeMap.stories.tsx:6:1)
      at Object.<anonymous> (lib/components/TreeMap/TreeMap.test.tsx:5:1)

It's okay to migrate to ES Modules but if you are dropping support for CommonJS I believe that the best thing to do is to publish a new major version since this is most likely a breaking change.

Also, if someone knows how to fix this so that Jest is able to process these modules, please let me know.

vasturiano commented 1 year ago

@joaosamouco thanks for reaching out.

One of the motivations for the full migration to ES modules is that the CommonJS entrypoint in this package was already not functioning properly, because some of the sub-dependencies were already not CommonJS compatible; such as d3-scale for example.

Thus, it wasn't a major version bump because in fact the feature was already broken, and the package was falsely advertising that it could be safely bound as a CommonJS module.

In your case I'd suggest pinning to v1.6.0 if you really need the explicit CommonJS binding. But the future-proof way is to migrate to using ES modules.

Is the Jest (experimental) ESM support not functioning well enough in this case?

joaosamouco commented 1 year ago

Hi!

In your case I'd suggest pinning to v1.6.0 if you really need the explicit CommonJS binding. But the future-proof way is to migrate to using ES modules.

This won't work since icicle-chart is not using fixed dependencies versions so even older versions of icicle-chart will still install more recent versions of kapsule and float-tooltip for example. Only a major version would prevent this from happening. (I'm not using package-lock.json to lock all the dependencies so that's also my bad.

Is the Jest (experimental) ESM support not functioning well enough in this case?

After some trial and error I was finally to make it work, I will leave the instructions in case it is helpful to someone else.

  1. Install @babel/core, babel-jest and @babel/plugin-transform-modules-commonjs
    npm i -D @babel/core babel-jest @babel/plugin-transform-modules-commonjs
  2. Update or create a babel.config.js file to use this transformer only when testing
    module.exports = {
    env: {
    test: {
      plugins: ['@babel/plugin-transform-modules-commonjs'],
    },
    },
    };
  3. Update jest.config.js to transform icicle-chart, accessor-fn, kapsule, and float-tooltip
    // jest.config.js
    module.exports = () => ({
    ...
    transform: {
    ...
    // support for ES Modules
    'node_modules/(float-tooltip|kapsule|icicle-chart|accessor-fn)/.+\\.m?jsx?$': 'babel-jest',
    },
    // ignore all node_modules except the ones we are transforming
    transformIgnorePatterns: [
    'node_modules/(?!(float-tooltip|kapsule|icicle-chart|accessor-fn)/)',
    ],
    });
vasturiano commented 1 year ago

@joaosamouco I'm surprised you didn't already have this issue with all the d3-* dependencies. Their latest major version is also ESM only. Did you not have to include them as exceptions in your Jest config?

joaosamouco commented 1 year ago

@joaosamouco I'm surprised you didn't already have this issue with all the d3-* dependencies. Their latest major version is also ESM only. Did you not have to include them as exceptions in your Jest config?

Oh, I see. For d3-* I had a different solution based on this issue https://github.com/facebook/jest/issues/12036. The same solution could also work in this case.

// jest.config.js
module.exports = () => ({
  ...
  moduleNameMapper: {
    ...,
    '^d3-(.*)$':
      '<rootDir>/node_modules/d3-$1/dist/d3-$1.min.js'
    '^(icicle-chart|float-tooltip|kapsule|accessor-fn)$':
      '<rootDir>/node_modules/$1/dist/$1.min.js',
  },
});