Loading @carbon/charts-react via require() gives internal error of ‘__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED’.

wkeese commented 2 months ago

(edited with latest information)

Application/Team

IKC

What happened?

After trying to upgrade to @carbon/charts-react 1.15.7, I’m getting a TypeError of Cannot read properties of undefined (reading ‘__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED’)

For me, the problem happens creating a SimpleBarChart. It also happens with LineChart.

I see it’s something in react-jsx-runtime.production.min.js, which for some reason is bundled into @carbon/charts-react/dist/index.js (and also index.mjs).

Apparently, the workaround is to downgrade to 1.6.14.

Version

@carbon/charts 1.15.7 @carbon/charts-react 1.15.7 react 18.3.1 react-dom 18.3.1

Data & options used

<SimpleBarChart
    data={[
      {
        group: "Qty",
        value: 65000,
      },
      {
        group: "More",
        value: 29123,
      },
      {
        group: "Sold",
        value: 35213,
      },
      {
        group: "Restocking",
        value: 51213,
      },
      {
        group: "Misc",
        value: 16932,
      },
    ]}
    options={{
      title: "Horizontal simple bar (discrete)",
      axes: {
        left: {
          mapsTo: "group",
          // scaleType: 'labels'
        },
        bottom: {
          mapsTo: "value",
        },
      },
      height: "400px",
    }}
  />

Relevant log output

TypeError
Cannot read properties of undefined (reading '__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED')
Call Stack
 undefined
  metadataimports.chunk.vendors-node_modules_carbon_icons-react_lib_activity_16_js-node_modules_carbon_icons-react_li-7d43f9.js:23285:31
 LH
  metadataimports.chunk.vendors-node_modules_carbon_icons-react_lib_activity_16_js-node_modules_carbon_icons-react_li-7d43f9.js:23814:18
 undefined
  metadataimports.chunk.vendors-node_modules_carbon_icons-react_lib_activity_16_js-node_modules_carbon_icons-react_li-7d43f9.js:23817:38
 undefined
  metadataimports.chunk.vendors-node_modules_carbon_icons-react_lib_activity_16_js-node_modules_carbon_icons-react_li-7d43f9.js:2690:231
 ./node_modules/@carbon/charts-react/dist/index.js
  metadataimports.chunk.vendors-node_modules_carbon_icons-react_lib_activity_16_js-node_modules_carbon_icons-react_li-7d43f9.js:2691:11
 options.factory
  metadataimports.main.js:364708:31
 __webpack_require__
  metadataimports.main.js:364007:33
 fn
  metadataimports.main.js:364348:21
 ./src/routes/components/MetadataEnrichments/routes/components/DisplayEnrichment/EnrichmentAssets/OverviewTab/OverviewTile/BarChart/BarChart.tsx
  metadataimports.chunk.src_routes_components_MetadataEnrichments_routes_components_DisplayEnrichment_index_ts.js:7469:20
 options.factory
  metadataimports.main.js:364708:31
◀ Prev× CloseNext ▶

StackBlitz example

https://stackblitz.com/edit/carbon-charts-react-webpack-cjs-example

Reproduction

It only happens when loading via require(), either explicitly using require() or using @babel/plugin-transform-modules-commonjs. Also presumably when using @babel/preset-env with certain settings of module.

Unclear if it only happens with Webpack or not. I'm guessing it would happen without Webpack, although you do need babel to compile the JSX file into JS.

What priority level would this be in your opinion?

P0

When did it break?

I traced it down to breaking sometime between 1.13.8 and 1.13.18. Unfortunately lots of files changed between then, and I can't easily narrow it down further because the releases were completely broken from 1.13.9 - 1.13.17.

https://github.com/carbon-design-system/carbon-charts/compare/v1.13.8...v1.13.18

1.13.8 works 1.13.9 No matching version found for @carbon/charts-react@1.13.9 1.13.10 ~ 1.13.15 other build problems (Can't resolve '@carbon/charts-react’) 1.13.17 Error: Package path . is not exported from package /Users/bill/wkc/mdi/node_modules/@carbon/charts-react 1.13.18 breaks

Theories

Due to bundling react-jsx-runtime.production.min.js in the carbon-charts rollup. See https://github.com/carbon-design-system/carbon-charts/pull/1799.
Due to react and react-dom being listed as dependencies as well as peerDependencies. I ended up with multiple versions of react (and react-dom), in node_modules/react and node_modules/@carbon/charts/node_modules/react.

nstuyvesant commented 2 months ago

The example above only shows the usage of the component but not how you're importing or building the project.

Are you using webpack and the UMD version of @carbon/charts-react?

Also, seems like the source map is not displaying a useful stack trace. What browser are you using?

nstuyvesant commented 2 months ago

Also, are you able to try this with React 18.2.0?

wkeese commented 2 months ago

The example above only shows the usage of the component but not how you're importing or building the project. Are you using webpack and the UMD version of @carbon/charts-react?

Oh, we're using Webpack, it's loading @carbon/charts-react/dist/index.js or possibly index.mjs.

I'm seeing this on a development build; I haven't tried production.

Also, seems like the source map is not displaying a useful stack trace. What browser are you using?

I'm using Chrome.

I see that there's a @carbon/charts-react/dist/index.js.map file but I guess it's not working for us.

My advice is that you should stop trying to rollup your source into a single file. If you look at other distributions like @carbon itself they just distribute all the files (except converted from Typescript to Javascript).

You can release a UMD version too, but there's no reason to rollup the ESM version.

Having said that, I don't think it's the cause of this bug.

Also, are you able to try this with React 18.2.0?

Yes, I just tried it now (React 18.2.0, @carbon/charts 1.13.18, @carbon/charts-react 1.13.18). The error reproduces.

aaronsahlin commented 2 months ago

I am seeing the same error. The last working version for me is

    "@carbon/charts": "1.13.8",
    "@carbon/charts-react": "1.13.8",

and this error started with

    "@carbon/charts": "1.13.18",
    "@carbon/charts-react": "1.13.18",

app.js:63882 Uncaught TypeError: Cannot read properties of undefined (reading 'SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED') at eval (index.js:102:626) at LH (index.js:112:2839) at eval (index.js:112:2866) at eval (index.js:2:203) at eval (index.js:2:234) at ./node_modules/@carbon/charts-react/dist/index.js (app.js:49642:1) at options.factory (app.js:64515:31) at webpack_require__ (app.js:63879:33) at fn (app.js:64172:21) at eval (LineGraphWithLoader.js:11:1)

LineGraphWithLoader line 11 is the import for the chart
import { LineChart } from '@carbon/charts-react';

My microservice is using React version 17.0.2, and it's not a trivial change for me to upgrade to React 18.

We use webpack.

nstuyvesant commented 2 months ago

@aaronsahlin - the stack trace suggests webpack is loading @carbon/charts-react/dist/index.js which is the UMD bundle even though you are using an import (which is surprising but maybe expected for webpack).

What happens if you explicitly import from the ESM...

import { LineChart } from '@carbon/charts-react/dist/index.mjs';

I recall something about webpack not supporting certain types of package.json exports (I use vite which follows other conventions).

Also, why is your project also loading @carbon/charts@1.13.18? This should not need to be explicitly added as the styles and types are in @carbon/charts-react as well.

nstuyvesant commented 2 months ago

A question about your Webpack config... does it include mjs in resolve.extensions?

  resolve: {
    // Add `.mjs` to support ECMAScript Modules explicitly
    extensions: ['.mjs', '.js', '.json']
  },

aaronsahlin commented 2 months ago

@nstuyvesant

I 've always import both @carbon/charts and @carbon/charts-react, was it a requirement at one time? I'll switch to just importing charts-react.

I'm still trying to get our webpack configured for mjs files.... I currently am getting

ERROR in ./libs/vpc-client-monitoring/src/components/LineGraphWithLoader.js 3:424-470 Module not found: Error: Package path ./dist/index.mjs is not exported from package /Users/asahlin/BM/carbon-11-genesis-ui/genesis-ui/node_modules/@carbon/charts-react (see exports field in /Users/asahlin/BM/carbon-11-genesis-ui/genesis-ui/node_modules/@carbon/charts-react/package.json) @ ./libs/vpc-client-monitoring/src/components/Graph.js 3:1820-1852 @ ./libs/vpc-client-monitoring/src/containers/GraphContainer.js 3:616-646 @ ./libs/vpc-client-monitoring/src/pages/MonitoringView.js 3:2055-2094 @ ./libs/vpc-client-monitoring/src/containers/MonitoringViewContainer.js 3:526-560 @ ./libs/vpc-client-service-virtual-server/src/pages/VsMonitoring.js 3:1034-1111 @ ./libs/vpc-client-service-virtual-server/src/index.js 3:1228-1259 @ ./libs/vpc-client-service-shared/src/vpc-services.js 3:6276-6330 @ ./libs/vpc-client-service-shared/src/reducers/app.js 3:1680-1706 @ ./src/client/app.js 3:877-936

nstuyvesant commented 2 months ago

In the past, both were required because you would get the styles.css from @carbon/charts but now it's included with @carbon/charts-react. Also, the latter also exports most of the TypeScript types you may need now as well.

nstuyvesant commented 2 months ago

@aaronsahlin - Looking at https://unpkg.com/@carbon/charts-react@1.15.7/package.json, the exports are there for the index.mjs (module as well as exports.import). It seems like webpack is not reading package.json's module property).

Can you share the calculated webpack config as well as what version of webpack you are using?

Also, would you have the ability to experiment by doing a build with vite or rollup? Just trying to understand if the issue is that webpack is not getting the exports from the current package.json configuration.

aaronsahlin commented 2 months ago

We are using webpack version 5.89.0.

Here are the webpack config files we use. webpack.base.config imports webpack.dev.config webpackConfig.zip

Doing a build with vite or rollup not really an option for us.

nstuyvesant commented 2 months ago

@aaronsahlin - thanks for sharing your webpack configs. It looks like neither explicitly resolve mjs extensions per https://webpack.js.org/configuration/resolve/#resolveextensions.

The default for resolve.extensions appears to be ['.js', '.json', '.wasm'] which suggests 'mjs' is not included which may be causing the fallback to UMD - not 100% sure.

Could you try adding this line after line 77 in webpack.base.config.js (could also add after line 49 of webpack.dev.config.js)...

    extensions: ['.js', '.json', '.wasm', '.mjs'],

aaronsahlin commented 2 months ago

Thanks for the suggestion, but no change... after adding the extensions section in I still get the same error when using the standard import. import { LineChart } from '@carbon/charts-react';

app.js:63802 Uncaught TypeError: Cannot read properties of undefined (reading 'SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED') at eval (index.js:102:626) at LH (index.js:112:2839) at eval (index.js:112:2866) at eval (index.js:2:203) at eval (index.js:2:234) at ./node_modules/@carbon/charts-react/dist/index.js (app.js:49562:1) at options.factory (app.js:64435:31) at webpack_require__ (app.js:63799:33) at fn (app.js:64092:21) at eval (LineGraphWithLoader.js:11:1)

And if I try import { LineChart } from '@carbon/charts-react/dist/index'; (with or without the .mjs) I get

ERROR in ./libs/vpc-client-monitoring/src/components/LineGraphWithLoader.js 3:424-466 Module not found: Error: Package path ./dist/index is not exported from package /Users/asahlin/BM/carbon-11-genesis-ui/genesis-ui/node_modules/@carbon/charts-react (see exports field in /Users/asahlin/BM/carbon-11-genesis-ui/genesis-ui/node_modules/@carbon/charts-react/package.json)

nstuyvesant commented 2 months ago

In your webpack.base.config.js, can you add this line in resolve.alias...

      '@carbon/charts-react': path.resolve(projectDirectory, 'node_modules/@carbon/charts-react/dist/index.mjs'),

then change your import to:

import { LineChart } from '@carbon/charts-react'

wkeese commented 2 months ago

BTW, we're getting that same weird behavior where webpack pulls in the CJS/UMD files:

var _chartsReact = __webpack_require__(/*! @carbon/charts-react */ "./node_modules/@carbon/charts-react/dist/index.js");
var _charts = __webpack_require__(/*! @carbon/charts */ "./node_modules/@carbon/charts/dist/umd/bundle.umd.js");

Not sure why, your package.json file looks good to me. It even has the "exports" section, the modern way of specifying all the exports.

I tried adding that line to resolve.alias, but it didn't make any difference.

wkeese commented 2 months ago

PS: I figured out part of the mystery.

We are using the babel plugin called "@babel/plugin-transform-modules-commonjs". @aaronsahlin might be using it too, perhaps unknowingly, since it's included as part of @babel/preset-env.

The plugin converts our ESM code to CJS. And when Webpack sees the CJS version of our code (that calls require("@carbon/charts/...")), it pulls in the CJS (or actually UMD) versions of charts.

Why would anyone use that babel plugin? It sounds like a dumb idea, since ESM is the standard, tree shaking works better with ESM, etc. But I think it was done to support running the code on Node without using Webpack. In the old days, Node couldn't read ESM format (especially not if they have the .js extension instead of the .mjs extension).

I don't know if any of this is related to that React error though.

nstuyvesant commented 2 months ago

@wkeese - That probably solved the mystery as to why the UMD bundle is getting loaded instead of ESM despite the webpack config being setup to handle mjs extensions. Out of curiosity, if you do not use @babel/plugin-transform-modules-commonjs, do you get a different result? The demo site for @carbon/charts-react uses ESM so I'm wondering if the problem is localized to the way we are building the UMD bundle. Both should work but if we find it's just UMD for those using webpack, it helps narrow things down a bit.

@aaronsahlin - not sure why the source map wasn't used to generate your stack trace in Chrome. Could you see if Firefox uses the source map for your error? I sometimes get better results with Firefox loading source maps - worth a shot.

aaronsahlin commented 2 months ago

@nstuyvesant I tried the alias suggestion, no change.

@wkeese We are specifying@babel/plugin-transform-modules-commonjs (7.23.3), along with @babel/preset-env (7.23.3). I tried removing just @babel/plugin-transform-modules-commonjs from our package.json and now my charts load! I am not 100% sure why since like you said its included as part of @babel/preset-env.

wkeese commented 2 months ago

@aaronsahlin - Nice! About @babel/preset-env, it turns out that although the transform is included in @babel/preset-env, but you have to enable it via the modules option. (I don't understand exactly how modules works, but it sounds like the conversion isn't happening in your case.)

PS: I wondering if this problem is related to the Dual package hazard.

nstuyvesant commented 2 months ago

@aaronsahlin - thanks for confirming! So it does sound like the issue is specific to the UMD bundle being used by Webpack and Common JS. That helps narrow things down quite a bit.

wkeese commented 2 months ago

FYI, likewise for me, after removing @babel/plugin-transform-modules-commonjs, the problem goes away, even with the 1.15.7 release.

Before changing that, I tried to trace down the exact commit where the problem started, but was getting contradictory results. Sometimes I traced it down to 766dcac941e0cf976c0d88ef57a3a225dad28038 (which seems likely), but other times I confirmed that it started with 8fcb5b96f5fc3dc5a5bb1cfb3f888bbbe41f6fc3 (which seems impossible). I guess it was a problem with caching but I tried to be careful about not caching anything. Or just an intermittent problem. It's unclear.

nstuyvesant commented 2 months ago

@wkeese - in the first commit you referenced, the main difference for the @carbon/charts-react package is the change in the vite.config.ts omission of...

    optimizeDeps: {
        disabled: true, // deprecated in more recent versions of vite
        include: ['@carbon/charts', '@carbon/icons-react'],
        exclude: [
            // Will cause errors when running storybook if in the include list
            '@carbon/telemetry'
        ]
    },

Other changes were reverted. The line of code raising your error is in the core package so it could be related to optimization default which are currently used.

wkeese commented 2 months ago

Hmm, I tried adding back the optimizeDeps. I also tried rolling back to the vite.config.ts from e4f6905218ec0f1a85799e32b124981311825893. The error still happens.

Have you guys tried running a web page using the UMD version of @carbon/charts-react? If that's failing, then you have something to investigate.

Otherwise, I'm not sure it's worth pursuing the cause of this failure. The problem only happens with UMD, and IMO no one should be using UMD anyway. @aaronsahlin and I were using it by accident. It's possible (but seems unlikely) that other people are using it on purpose. But you can wait to see if they are getting problems with the latest code.

FWIW, I don't think you should be creating a rollup for ESM (using vite, webpack, or rollup), as it doesn't really achieve anything and it just makes tree-shaking and debugging harder. (I'm assuming that applications consuming carbon-charts are using Webpack, Vite etc.) But your ESM rollup is not causing any immediate problems that I know of (including the one in this ticket).

wkeese commented 2 months ago

PS: I think you asked me or @aaronsahlin why we were directly importing @carbon/charts. For me, it was to get some type definitions / enums that you didn't export from @carbon/charts-react. Specifically BarEvent. IIRC you only export a curated list of the types.

import { SimpleBarChart } from "@carbon/charts-react";
import { BarEvent, ChartTheme, ScaleTypes } from "@carbon/charts";
...
  useEffect(() => {
    const _barOnClick = ({ detail }: BarClickEvent) => barOnClick(detail.datum);
    const refCurrent = barChartRef?.current;
    if (refCurrent?.chart) {
      refCurrent.chart.services.events.addEventListener(
        BarEvent.BAR_CLICK,
        _barOnClick
      );
    }
...

nstuyvesant commented 2 months ago

@wkeese - Thanks for the feedback. A few comments...

There is a UMD bundle for @carbon/charts. Here's a StackBlitz example: https://stackblitz.com/edit/kriqyk?file=index.html. One use-case is Salesforce Lightning where you don't use bundlers like webpack, rollup or vite.
I tend to agree that the UMD bundle for @carbon/charts-react should not be used because you should use ESM in 2024 and if you have a use-case for UMD, the one for @carbon/charts probably has it covered.
The problem with the @carbon/charts-react UMD and webpack might have crept in with a vite version update and some of the changes to its defaults over time.
Regarding re-exporting enums from @carbon/charts to the other packages, I've added all enums to a PR that is under review. Thanks for showing me those gaps. While I'm not re-exporting everything at this point, definitely makes sense to have enums.

Given your feedback, what are your thoughts about closing this issue?

wkeese commented 2 months ago

There is a UMD bundle for @carbon/charts. Here's a StackBlitz example: https://stackblitz.com/edit/kriqyk?file=index.html. One use-case is Salesforce Lightning where you don't use bundlers like webpack, rollup or vite.

Thanks for the response. But I was talking about a CJS example for @carbon/charts-react, not a CJS/UMD example for vanilla @carbon/charts.

I reproduced the problem in https://stackblitz.com/edit/carbon-charts-react-webpack-cjs-example?file=src%2Findex.html. I'm seeing the error in the console there. Do you see it?

I tend to agree that the UMD bundle for @carbon/charts-react should not be used because you should use ESM in 2024 and if you have a use-case for UMD, the one for @carbon/charts probably has it covered.

I guess the problem is that your package.json points to that UMD bundle, and it gets used by accident (see above test case) by anyone who is unfortunately still using CJS.

Regarding re-exporting enums from @carbon/charts to the other packages, I've added all enums to a PR that is under review. Thanks for showing me those gaps.

Thanks!

theiliad commented 2 months ago

should we point to a non-mjs entry by default, but keep mjs as a module entry?

nstuyvesant commented 2 months ago

@theiliad - if by default you mean package.json's main, it points to the non-mjs (the UMD)...

    "type": "module",
    "main": "./dist/index.js",
    "module": "./dist/index.mjs",
    "types": "./dist/index.d.ts",
    "exports": {
        "./package.json": "./package.json",
        ".": {
            "types": "./dist/index.d.ts",
            "import": "./dist/index.mjs",
            "require": "./dist/index.js"
        },
        "./styles.min.css": "./dist/styles.min.css",
        "./styles.css": "./dist/styles.css"
    },

There were two issues (@wkeese and @aaronsahlin - please correct anything here that looks incorrect)...

In @wkeese and @aaronsahlin's cases, they did not need to use @babel/plugin-transform-modules-commonjs. Using it forced the UMD for @carbon/charts-react to be loaded (not an MJS issue since it has a .js extension and it was loading it). Removing @babel/plugin-transform-modules-commonjs solved their problems but...
Their configuration revealed an issue with @carbon/charts-react UMD that's not extension related. Something about the way vite is building it is problematic for someone using CJS with @carbon/charts-react. I verified the UMD for @carbon/charts works fine (built with the same options). So this appears to be an issue of the React UMD built by vite and incorporated into a project that uses React, Webpack, and Common JS.

aaronsahlin commented 2 months ago

For my case, I may be able to remove @babel/plugin-transform-modules-commonjs, we are a larger microservice and I would have to dig into why we were using it to begin with to see if it can just be removed. (but correct removing it did fix my issue).

nstuyvesant commented 2 months ago

@wkeese - Thanks for creating the repro on StackBlitz at https://stackblitz.com/edit/carbon-charts-react-webpack-cjs-example?file=src%2Findex.html.

When I load this using Chrome, I see the error you reported.

So I tried creating my own StackBlitz example where I use Common JS to load the chart. This example uses @carbon/charts-react UMD but not Webpack and it works... https://stackblitz.com/edit/react-pntewt?file=src%2Findex.js

wkeese commented 2 months ago

This example uses @carbon/charts-react UMD

Actually I disagree, it looks to me like Vite is pulling in the ESM version @carbon/charts-react. Notice the import and export statements:

If you really want to narrow down if it's a Webpack problem, I suggest writing a test without Webpack or Vite, just transpiling the JSX into JS (with Babel), and then it directly instantiates the chart with the @carbon/charts-react UMD package.

I guess you would need to include RequireJS for the main index.jsx file to be able to load the charts-react UMD bundle.

nstuyvesant commented 2 months ago

Hi @wkeese - While StackBlitz has a React Vite template (which uses a WebContainer like your example), the example I shared used the create-react-app which does not. While you can browse the src folder for @carbon/charts-react in Sources, what is loaded is dist/index.js which is UMD (and displayed here using the source map - ESM would have been index.mjs)...

Had it loaded the ESM version, the file would have been dist/index.mjs which looks like this (without the source map since I just opened it in VS Code)...

Just add a breakpoint to the UMD code and reload the preview in StackBlitz and you'll see what I mean.

Regarding StackBlitz's create-react-app template, it looks like they are using Webpack. Please see https://github.com/stackblitz/create-react-app-template.

wkeese commented 2 months ago

Ah right, I stand corrected. Well, maybe it is due to Webpack.

It's still hard to tell for me. For example, my failing test case loads the CJS version of React (node_modules/react/cjs/react.development.js), whereas in your test case, I can't tell which version it's loading.

carbon-design-system / carbon-charts