fix(1522): externalize dom-to-image-more #1540

nstuyvesant · 2023-04-13T18:59:39Z

Updates

Externalizes dom-to-image to solve issue 1522. Problem occurs frequently when working in SvelteKit when running vite dev when there are imports like enums from charts core package.
Adds classnames dependency for charts-react (was missing - not sure why)
Adds missing parameters for three functions that are required given the TypeScript interface definitions.
No longer requires yarn dependencies to be offline - discourages contributors because it pushes up storage requirements for forks.

Review checklist (for reviewers only)

netlify · 2023-04-13T18:59:46Z

✅ Deploy Preview for carbon-charts-react ready!

Name	Link
🔨 Latest commit	`020a305`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-charts-react/deploys/64414e70ff999b0008f896a8
😎 Deploy Preview	https://deploy-preview-1540--carbon-charts-react.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify · 2023-04-13T18:59:46Z

✅ Deploy Preview for carbon-charts-angular ready!

Name	Link
🔨 Latest commit	`020a305`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-charts-angular/deploys/64414e70ff999b0008f896ad
😎 Deploy Preview	https://deploy-preview-1540--carbon-charts-angular.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify · 2023-04-13T18:59:50Z

✅ Deploy Preview for carbon-charts-core ready!

Name	Link
🔨 Latest commit	`020a305`
🔍 Latest deploy log	https://app.netlify.com/sites/carbon-charts-core/deploys/64414e7019a6de0008bedeee
😎 Deploy Preview	https://deploy-preview-1540--carbon-charts-core.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

theiliad

LGTM

nstuyvesant · 2023-04-13T19:24:09Z

Hoisting issue resolved and corrected a few other minor issues blocking the build (overly sensitive typescript settings from missing parameters, missing dependency for charts-react). Builds successfully now. Just pushed those updates.

nstuyvesant · 2023-04-13T20:21:59Z

Now we're good!

nstuyvesant · 2023-04-19T13:11:01Z

Hi @theiliad - Got several other PRS I wanted to submit (Storybook, vite everywhere, etc.) but was waiting for this one to get merged (as it gets rid of the offline packages). Any thoughts as to when this one can be merged?

theiliad · 2023-04-19T13:37:07Z

Hi @theiliad - Got several other PRS I wanted to submit (Storybook, vite everywhere, etc.) but was waiting for this one to get merged (as it gets rid of the offline packages). Any thoughts as to when this one can be merged?

Hi @nstuyvesant the issue right now is the .yarn/cache directory. Can we remove that? Because of that I can't see the full changes either, my browser just lags up 🙂

…, missing params)

theiliad

😓
I think there's an issue with yarn now on Netlify https://app.netlify.com/sites/carbon-charts-core/deploys/6440203d8a33c40008a82df5

This might be because of the .yarnrc.yml change?

…che (yet)

nstuyvesant · 2023-04-19T17:18:25Z

Hi @nstuyvesant the issue right now is the .yarn/cache directory. Can we remove that? Because of that I can't see the full changes either, my browser just lags up 🙂

Just dropped two of the commits from this PR. That should resolve your issue. In general, I would like to get rid of the yarn-offline-mirror-cache from the repo as it's a storage challenge and makes contributing problematic.

The modified .gitignore but did not touch the directory for now. Perhaps a separate PR could be created to resolve some of the workspace issues around yarn and lerna?

theiliad · 2023-04-19T18:19:46Z

Hi @nstuyvesant the issue right now is the .yarn/cache directory. Can we remove that? Because of that I can't see the full changes either, my browser just lags up 🙂

Just dropped two of the commits from this PR. That should resolve your issue. In general, I would like to get rid of the yarn-offline-mirror-cache from the repo as it's a storage challenge and makes contributing problematic.

The modified .gitignore but did not touch the directory for now. Perhaps a separate PR could be created to resolve some of the workspace issues around yarn and lerna?

It seems like builds are still showing the same error.

Why would we drop yarn-offline-mirror-cache? They seem to be working well... any specific reasons?

nstuyvesant · 2023-04-19T19:01:56Z

Not all contributors have the available storage to keep all the dependencies for a fork in their GitHub account.

theiliad · 2023-04-19T19:23:53Z

Not all contributors have the available storage to keep all the dependencies for a fork in their GitHub account.

Oh interesting! I didn't know that. How does that happen?

nstuyvesant · 2023-04-19T21:53:06Z

Oh interesting! I didn't know that. How does that happen?

Let's say I update a bunch of dependencies or change the yarn version (which switches the cache from .tgz to .zip and changes all the cached dependencies), then I am incurring the storage for all the changed dependencies in my fork. This monorepo has a large set of dependencies so it's more likely to tip me over my limit. If others want to contribute, they will face the same thing if they change lots of dependencies on their forks.

Including the yarn-offline-mirror in the repo is like including node_modules. All of that can be reconstituted with a simple yarn install.

Also, yarn 1.22.19 is the last version of yarn to use the .yarnrc (without the .yml extension). The options for yarn-offline-mirror and yarn-offline-mirror-pruning no longer exist post 1.22.19.

nstuyvesant · 2023-04-19T22:14:57Z

@theiliad - just pushed some small changes that keep yarn at 1.22.19 (for now) but solve problems for contributors while laying the foundation for a yarn upgrade (newer .yarnrc.yml format, packageManager in package.json, simplified workspace property in package.json).

Please let me know if you have any concerns or questions. After these items are merged, it will be much easier to cherry-pick non-breaking changes from my next branch.

theiliad

Awesome! Only thing I noticed is the contents of .yarn/releases/yarn-1.22.19.cjs seems weird, there are some weird symbols showing up in there.

Can we use the JS file available here instead?
https://github.com/yarnpkg/yarn/releases/tag/v1.22.19

I could also update it if you add me as a collab to your fork

theiliad · 2023-04-20T01:36:47Z

Awesome! Only thing I noticed is the contents of .yarn/releases/yarn-1.22.19.cjs seems weird, there are some weird symbols showing up in there.

Actually for licensing purposes, are we able to avoid having that file in the repo?

Other than that looks good to me 💯 thanks for working on these

nstuyvesant · 2023-04-20T03:22:34Z

Awesome! Only thing I noticed is the contents of .yarn/releases/yarn-1.22.19.cjs seems weird, there are some weird symbols showing up in there.

Actually for licensing purposes, are we able to avoid having that file in the repo?

Other than that looks good to me 💯 thanks for working on these

Hi again. That file represents the public sources to yarn 1.22.19. I didn't see any odd characters myself but it is very long. The license is referenced on line 1979. Many public repos have these like:

Babel (https://github.com/babel/babel/tree/main/.yarn/releases)
Angular (https://github.com/angular/angular/tree/main/.yarn/releases)
Storybook (https://github.com/storybookjs/storybook/tree/next/.yarn/releases)

The newer .yarnrc.yml expects the yarn sources to be there (and part of the project). If it's removed, the reference to it would also have to be removed from .yarnrc.yml but yarn versions after 1.22.19 will require it.

Here's a Q&A on the yarn website about not gitignoring the .yarn/releases folder... https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored.

If you are using modern yarn (versions 2.0 to 3.5.0), here's the install process: https://yarnpkg.com/getting-started/install. Once performed you can set a project to use 1.22.19 (or apply it globally - switching between versions is trivial).

The yarn release file you found was created by the command, yarn set version 1.22.19 after modern yarn was installed.

Hope this helps!

theiliad · 2023-04-20T14:47:56Z

Thanks @nstuyvesant 💯

MRumpold · 2023-04-22T09:33:21Z

Hi,
I believe the bug fix you mentioned is causing an issue in my project. (I'm using SvelteKit, started with vite dev as mentioned at the very beginning). When I call a page with an embedded chart, the following message appears on the console:

X [ERROR] Could not resolve "dom-to-image-more"

    node_modules/.pnpm/@carbon+charts@1.7.1_d3@7.8.4_sass@1.62.0/node_modules/@carbon/charts/services/essentials/dom-utils.js:24:23:
      24 │ import domToImage from 'dom-to-image-more';
         ╵                        ~~~~~~~~~~~~~~~~~~~

  You can mark the path "dom-to-image-more" as external to exclude it from the bundle, which will remove this error.

A vite build fails with the following error message:

[vite]: Rollup failed to resolve import "dom-to-image-more" from "D:/.../node_modules/.pnpm/@carbon+charts@1.7.1_d3@7.8.4_sass@1.62.0/node_modules/@carbon/charts/services/essentials/dom-utils.js".`

These are my current Carbon dependencies from the package.json:

"carbon-components-svelte": "^0.72.3",
"carbon-icons-svelte": "^11.4.0",
"carbon-preprocess-svelte": "^0.9.1",
"@carbon/charts": "^1.7.1",
"@carbon/charts-svelte": "^1.7.1",

I have already tried a lot including:

clearing the pnpm cache
installing the dom-to-image-more package -> but it resulted in more errors.

I'm not sure if this is a bug or a combination with the configuration.

Can someone please help me to get the project running again?

nstuyvesant · 2023-04-22T12:16:05Z

If you are using rollup...

export default {
  // ...
  external: ['dom-to-image-more'],
  // ...
}

For vite...

/** @type {import('vite').UserConfig} */
const config = {
	build: {
		rollupOptions: {
			external: ['dom-to-image-more']
		}
	}
// ...

Can potentially do the exclusion in the charts-svelte package itself. Previously, dom-to-image was part of the core project and caused problems with SvelteKit when running dev (vite). Now that it's externalized, it's easier to mitigate (with the two examples above).

nstuyvesant · 2023-04-22T12:45:18Z

I just submitted #1545 to simplify things for projects and packages that consume @carbon/charts but what I provided will immediately solve the problem at your project level. Note: the problem was there previously and would show up as an error when running in vite with SvelteKit.

MRumpold · 2023-04-22T13:54:56Z

Thank you very much, it works now with these settings.

MRumpold · 2023-04-26T16:13:00Z

Hello,

I was a bit too hasty, unfortunately it does not always work.
In vite.config.ts I added the following code as mentioned above:
build: {
rollupOptions: {
external: ['dom-to-image-more']
}
}

I installed dom-to-image-more as a module, and added the following dependency to the package.json file:
"dom-to-image-more": "^3.1.5"

It works when I start it with vite dev, but not with vite preview or vite build. The browser console shows the following error message:
"TypeError: Failed to resolve module specifier "dom-to-image-more".
Relative references must start with either "/", "./", or "../"."

Do you have any suggestions on how to fix this error?
Thank you for your help!

nstuyvesant · 2023-04-26T20:53:50Z

That is odd. On a separate note, there is a conflict between rollup and dom-to-image-more that I hope will be resolved by tomorrow on the dom-to-image-more side. At that point, I have a PR that will bundle dom-to-image-more within @carbon/charts.

As a temporary solution, perhaps downgrade to 1.6.14 until this is resolved. It should not take long.

nstuyvesant · 2023-04-27T03:21:28Z

The update to dom-to-image-more was released this evening. This PR, once merged, should resolve your issue...
#1545

fix(1522): externalize dom-to-image-more

cb6fbef

nstuyvesant requested review from theiliad, Akshat55 and a team as code owners April 13, 2023 18:59

nstuyvesant requested review from zvonimirfras and removed request for a team April 13, 2023 18:59

theiliad approved these changes Apr 13, 2023

View reviewed changes

nstuyvesant added 3 commits April 19, 2023 13:04

chore(netlify)

0b15f11

fix(build): small issues that were blocking build (missing dependency…

1edd12c

…, missing params)

chore(gitignore): don't include the 2 yarn caches in the repo

99927c3

nstuyvesant force-pushed the issue-1522-3 branch from c976fe6 to 99927c3 Compare April 19, 2023 17:09

theiliad requested changes Apr 19, 2023

View reviewed changes

chore(dom-to-image): add latest, don't exclude yarn-offline-mirror-ca…

9ebb521

…che (yet)

nstuyvesant added 4 commits April 19, 2023 18:03

delete yarn-offline-mirror-cache and move to .yarnrc.yml format

7cfb382

yarn config change

dbba428

.gitignore update

05ce40a

yarn workspace config in package.json

de219f3

theiliad requested changes Apr 20, 2023

View reviewed changes

Delete yarn-1.22.19.cjs

26bb022

Create yarn-1.22.19.cjs

020a305

theiliad merged commit 98f1bfc into carbon-design-system:master Apr 20, 2023
4 checks passed

theiliad added type: enhancement 💡 affects: build-process package: react package: svelte labels Apr 20, 2023

theiliad added this to Done in Q3 - 2023 Apr 20, 2023

nstuyvesant deleted the issue-1522-3 branch April 20, 2023 15:31

huineng mentioned this pull request Apr 24, 2023

[Bug] Build Error: Module not found: Error: Can't resolve 'dom-to-image-more' #1546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(1522): externalize dom-to-image-more #1540

fix(1522): externalize dom-to-image-more #1540

nstuyvesant commented Apr 13, 2023 •

edited

netlify bot commented Apr 13, 2023 •

edited

netlify bot commented Apr 13, 2023 •

edited

netlify bot commented Apr 13, 2023 •

edited

theiliad left a comment

nstuyvesant commented Apr 13, 2023 •

edited

nstuyvesant commented Apr 13, 2023

nstuyvesant commented Apr 19, 2023

theiliad commented Apr 19, 2023

theiliad left a comment

nstuyvesant commented Apr 19, 2023

theiliad commented Apr 19, 2023

nstuyvesant commented Apr 19, 2023

theiliad commented Apr 19, 2023

nstuyvesant commented Apr 19, 2023

nstuyvesant commented Apr 19, 2023

theiliad left a comment •

edited

theiliad commented Apr 20, 2023 •

edited

nstuyvesant commented Apr 20, 2023 •

edited

theiliad commented Apr 20, 2023

MRumpold commented Apr 22, 2023 •

edited

nstuyvesant commented Apr 22, 2023 •

edited

nstuyvesant commented Apr 22, 2023 •

edited

MRumpold commented Apr 22, 2023

MRumpold commented Apr 26, 2023 •

edited

nstuyvesant commented Apr 26, 2023

nstuyvesant commented Apr 27, 2023

fix(1522): externalize dom-to-image-more #1540

fix(1522): externalize dom-to-image-more #1540

Conversation

nstuyvesant commented Apr 13, 2023 • edited

Updates

Review checklist (for reviewers only)

netlify bot commented Apr 13, 2023 • edited

✅ Deploy Preview for carbon-charts-react ready!

netlify bot commented Apr 13, 2023 • edited

✅ Deploy Preview for carbon-charts-angular ready!

netlify bot commented Apr 13, 2023 • edited

✅ Deploy Preview for carbon-charts-core ready!

theiliad left a comment

Choose a reason for hiding this comment

nstuyvesant commented Apr 13, 2023 • edited

nstuyvesant commented Apr 13, 2023

nstuyvesant commented Apr 19, 2023

theiliad commented Apr 19, 2023

theiliad left a comment

Choose a reason for hiding this comment

nstuyvesant commented Apr 19, 2023

theiliad commented Apr 19, 2023

nstuyvesant commented Apr 19, 2023

theiliad commented Apr 19, 2023

nstuyvesant commented Apr 19, 2023

nstuyvesant commented Apr 19, 2023

theiliad left a comment • edited

Choose a reason for hiding this comment

theiliad commented Apr 20, 2023 • edited

nstuyvesant commented Apr 20, 2023 • edited

theiliad commented Apr 20, 2023

MRumpold commented Apr 22, 2023 • edited

nstuyvesant commented Apr 22, 2023 • edited

nstuyvesant commented Apr 22, 2023 • edited

MRumpold commented Apr 22, 2023

MRumpold commented Apr 26, 2023 • edited

nstuyvesant commented Apr 26, 2023

nstuyvesant commented Apr 27, 2023

nstuyvesant commented Apr 13, 2023 •

edited

netlify bot commented Apr 13, 2023 •

edited

netlify bot commented Apr 13, 2023 •

edited

netlify bot commented Apr 13, 2023 •

edited

nstuyvesant commented Apr 13, 2023 •

edited

theiliad left a comment •

edited

theiliad commented Apr 20, 2023 •

edited

nstuyvesant commented Apr 20, 2023 •

edited

MRumpold commented Apr 22, 2023 •

edited

nstuyvesant commented Apr 22, 2023 •

edited

nstuyvesant commented Apr 22, 2023 •

edited

MRumpold commented Apr 26, 2023 •

edited