icicle-chart icon indicating copy to clipboard operation
icicle-chart copied to clipboard

Migration to ES Modules broke Jest unit testing

Open joaosamouco opened this issue 2 years ago • 4 comments

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.

joaosamouco avatar Feb 07 '23 17:02 joaosamouco

@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?

vasturiano avatar Feb 07 '23 18:02 vasturiano

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
  1. 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'],
    },
  },
};
  1. 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)/)',
  ],
});

joaosamouco avatar Feb 08 '23 09:02 joaosamouco

@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?

vasturiano avatar Feb 08 '23 12:02 vasturiano

@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',
  },
});

joaosamouco avatar Feb 08 '23 13:02 joaosamouco