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

Specify CJS vs ESM exports for @nivo/colors package #2246

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

D-Maher
Copy link

@D-Maher D-Maher commented Feb 20, 2023

TypeScript has difficulty in trying to import @nivo/colors as an ESM module. This is because "main" in @nivo/colors package.json is specified as "./dist/nivo-colors.cjs.js".

This PR adds an exports field to the @nivo/colors package.json in order for TypeScript to be able to resolve the ESM module when declaring:

import { getContinuousColorScale } from '@nivo/colors';

If this is a welcome change, I am happy to update other Nivo packages accordingly.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b0a29a2:

Sandbox Source
nivo Configuration

@GeospatialMax
Copy link

Thanks for the PR @D-Maher . Do you reckon this would resolve the issue we're having when running our application in vitest as well (i.e. in a node environment).

Error: require() of ES Module /home/mstephan/gitRepos/realyse-platform/node_modules/d3-scale/src/index.js from /home/mstephan/gitRepos/realyse-platform/node_modules/@nivo/legends/dist/nivo-legends.cjs.js not supported.
Instead change the require of index.js in /home/mstephan/gitRepos/realyse-platform/node_modules/@nivo/legends/dist/nivo-legends.cjs.js to a dynamic import() which is available in all CommonJS modules.

@D-Maher
Copy link
Author

D-Maher commented Feb 23, 2023

Do you reckon this would resolve the issue we're having when running our application in vitest as well (i.e. in a node environment).

@GeospatialMax That is my hope. In fact, I submitted this PR to address that very same issue when running my app in Vitest. When I run my tests, I am seeing a similar error related to @nivo/core when using Yarn PnP:

Error: require() of ES Module my-project/.yarn/cache/d3-interpolate-npm-3.0.1-77ddca7977-8.zip/node_modules/d3-interpolate/src/index.js from my-project/.yarn/__virtual__/@nivo-core-virtual-99176c25b3/0/cache/@nivo-core-npm-0.80.0-e4f763d5ca-8.zip/node_modules/@nivo/core/dist/nivo-core.cjs.js not supported.
Instead change the require of index.js in my-project/.yarn/__virtual__/@nivo-core-virtual-99176c25b3/0/cache/@nivo-core-npm-0.80.0-e4f763d5ca-8.zip/node_modules/@nivo/core/dist/nivo-core.cjs.js to a dynamic import() which is available in all CommonJS modules.

Looks like a variety of Nivo packages result in errors like this, not just @nivo/colors.

@GeospatialMax
Copy link

FYI @D-Maher :
The approach you used above didn't quite work for me (plus it relies on Nivo updating their packages).

I managed to resolve this by adding this to my test configuration (in my case in vite.config.ts - based on this comment)

test: {
    alias: [
        { find: '@nivo/annotations', replacement: '@nivo/annotations/dist/nivo-annotations.es.js' },
        { find: '@nivo/arcs', replacement: '@nivo/arcs/dist/nivo-arcs.es.js' },
        { find: '@nivo/axes', replacement: '@nivo/axes/dist/nivo-axes.es.js' },
        { find: '@nivo/bar', replacement: '@nivo/bar/dist/nivo-bar.es.js' },
        { find: '@nivo/colors', replacement: '@nivo/colors/dist/nivo-colors.es.js' },
        { find: '@nivo/core', replacement: '@nivo/core/dist/nivo-core.es.js' },
        { find: '@nivo/legends', replacement: '@nivo/legends/dist/nivo-legends.es.js' },
        { find: '@nivo/line', replacement: '@nivo/line/dist/nivo-line.es.js' },
        { find: '@nivo/pie', replacement: '@nivo/pie/dist/nivo-pie.es.js' },
        { find: '@nivo/recompose', replacement: '@nivo/recompose/dist/nivo-recompose.es.js' },
        { find: '@nivo/scales', replacement: '@nivo/scales/dist/nivo-scales.es.js' },
        { find: '@nivo/scatterplot', replacement: '@nivo/scatterplot/dist/nivo-scatterplot.es.js' },
        { find: '@nivo/tooltip', replacement: '@nivo/tooltip/dist/nivo-tooltip.es.js' },
        { find: '@nivo/voronoi', replacement: '@nivo/voronoi/dist/nivo-voronoi.es.js' },
      ],
      ....

This ensures that the es modules are used instead of the cjs modules and prevents errors with further dependencies downstream.

@arnamak
Copy link

arnamak commented Feb 27, 2023

@GeospatialMax Curious, I like this approach you've got here, but it doesn't seem to work for me (whereas the PR variant does work).

Thinking maybe the difference is how projects are set up--do you have a standard node_modules configuration or are you using Yarn PnP? In my case it's the latter, maybe that's the kicker?

@plouc no rush, just wondering if you maybe have some thoughts.

@GeospatialMax
Copy link

@arnamak Interesting, currently still using standard node_modules approach (as the "new" PnP approach was failing in other places). So that might indeed make the difference.

@cmark1302
Copy link

facing same issue with PnP

@cmark1302
Copy link

cmark1302 commented Mar 24, 2023

@GeospatialMax I just fixed the issue for PnP with below code

test: {
    alias: {
      '@nivo/pie': require.resolve('@nivo/pie/dist/nivo-pie.es.js'),
    }
}

@eholt201
Copy link

finding nivo unusable with create-t3-app due to this issue, any suggestions?

@erquhart
Copy link

erquhart commented Jul 6, 2023

@plouc this would be a welcome change

@jedrzejginter
Copy link

@plouc Can we get this merged please 🙏

Copy link

stale bot commented Mar 13, 2024

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the stale label Mar 13, 2024
@Akkuma
Copy link

Akkuma commented Apr 12, 2024

bump

@stale stale bot removed the stale label Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants