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

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

Open
wkeese opened this issue Apr 30, 2024 · 32 comments

Comments

@wkeese
Copy link
Contributor

wkeese commented Apr 30, 2024

(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.

v1.13.8...v1.13.18

1.13.8 works
1.13.9 No matching version found for @carbon/[email protected]
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 fix(react): remove react-jsx-runtime from bundles #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
Copy link
Contributor

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
Copy link
Contributor

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

@wkeese
Copy link
Contributor Author

wkeese commented May 1, 2024

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
Copy link

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
Copy link
Contributor

nstuyvesant commented May 1, 2024

@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/[email protected]? This should not need to be explicitly added as the styles and types are in @carbon/charts-react as well.

@nstuyvesant
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

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
Copy link
Contributor

nstuyvesant commented May 1, 2024

@aaronsahlin - Looking at https://unpkg.com/@carbon/[email protected]/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
Copy link

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
Copy link
Contributor

@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
Copy link

aaronsahlin commented May 1, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

wkeese commented May 2, 2024

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
Copy link
Contributor Author

wkeese commented May 2, 2024

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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor Author

wkeese commented May 2, 2024

@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
Copy link
Contributor

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

@theiliad theiliad changed the title [Bug]: __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED error Webpack | @carbon/charts-react | UMD bundle | @babel/plugin-transform-modules-commonjs May 2, 2024
@wkeese
Copy link
Contributor Author

wkeese commented May 3, 2024

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 766dcac (which seems likely), but other times I confirmed that it started with 8fcb5b9 (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
Copy link
Contributor

@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
Copy link
Contributor Author

wkeese commented May 4, 2024

Hmm, I tried adding back the optimizeDeps. I also tried rolling back to the vite.config.ts from e4f6905. 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
Copy link
Contributor Author

wkeese commented May 4, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

wkeese commented May 5, 2024

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!

@wkeese wkeese changed the title Webpack | @carbon/charts-react | UMD bundle | @babel/plugin-transform-modules-commonjs Loading @carbon/charts-react via require() gives internal error of ‘__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED’. May 5, 2024
@theiliad
Copy link
Member

theiliad commented May 6, 2024

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

@nstuyvesant
Copy link
Contributor

@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)...

  1. 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...
  2. 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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor Author

wkeese commented May 6, 2024

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:

Screenshot 2024-05-07 at 8 38 13

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
Copy link
Contributor

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)...

image

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)...

image

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
Copy link
Contributor Author

wkeese commented May 7, 2024

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.

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

No branches or pull requests

4 participants