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

Get rid of node-canvas dependency #5735

Closed
finom opened this issue Jun 1, 2019 · 17 comments · Fixed by #5786
Closed

Get rid of node-canvas dependency #5735

finom opened this issue Jun 1, 2019 · 17 comments · Fixed by #5786

Comments

@finom
Copy link
Contributor

finom commented Jun 1, 2019

Some team members get lots of troubles while they install fabricjs because of the issues of installing node-canvas. I also got some troubles but I'm happy to be an macos user and it wasn't hard to fix it. Windows users aren't so happy with that :D

We use fabricjs only for browser-side development and we don't need any features for nodejs. Is that possible to remove "canvas" from "optionalDependencies", add it to "devDependencies" and leave a note at README that if somebody wants to use node-canvas then he/she needs to install it manually?

A good bonus would be also to get rid of "jsdom" dependency because it just gives a package more weight.

@asturur
Copy link
Member

asturur commented Jun 1, 2019

If you install with NPM and you use for the browser, you likely use a bundler.
Which is it?

Here is an example for webpack.
JSDOM would not be bundled with it, i use it in production since long.
https://github.com/fabricjs/fabricjs-webpack/blob/master/webpack.config.js

node-canvas 2 should install plain simple since is prebuilt for most systems.

I cannot get rid of the 2 dependencies or i would leave a package in npm that does not work by default and i do not like this idea.

I will close the issue as i did for similar bundling/installing issues, but i m very open to continue talking of your specific problem, we could both learn something.

@asturur asturur closed this as completed Jun 1, 2019
@finom
Copy link
Contributor Author

finom commented Jun 3, 2019

@asturur I use webpack but it doesn't actually matter here. The issue happens on npm install step. I'm an experienced trouble-shooter and I'm OK with issues I'd get with node-canvas install. But as I said that's painful for other team members who needs to do all this stuff described at node-canvas Wiki. I'd say there are two possible solutions of that:

  • Get rid of node-specific dependencies and just add a node that people need to install them manually (npm i fabric canvas jsdom).
  • Create an alternative NPM package which excludes node-specific dependencies.

I'm also thinking in case if you reject my proposal to create the alternative package by myself which is going to run a script before install which will pull this repository, modify package.json and install fabric.js without these dependencies. But I will loose versioning then...

@NazarKryvyy
Copy link

In our project, we are using webpack, react and serverside rendering, and after we had upgraded fabric to 3.1, we started to get problems with node-canvas. After deploying to the server, on server rendering we started to get the error 'Cannot find module '../build/Release/canvas.node' at module.js'. The strange thing is that there is no code connected with node-canvas in the bundled file. Another strange one is that the error disappears after several reloads of the page. After deploying, js file is being bundled only once, and after reloading the page it could not be changed, but suddenly the error had gone, and I cannot get why. Tried to use webpack config with configuring externals, that was mentioned earlier and it does not help. Helps only downgrading to 2.7 as it uses an earlier version of node-canvas. My guess is that in node-canvas has changed the way of installing in version 2.0. But why it is trying to be installed if it is optional dependency and was not required anywhere, still to be a question?

@asturur
Copy link
Member

asturur commented Jun 7, 2019

Optional dependency means that if it fails installing there are no problems.
So npm won't exit if node-canvas cannot get installed.

Can you post more of your webpack error?
You should probably add some code to avoid initializing fabric during ssr if you did not already, since that would be wasted effort on your server.

@asturur
Copy link
Member

asturur commented Jun 7, 2019

@finom i have a window machine here now, can you help me replicate a configuration where node-canvas is breaking an npm install?

@Pauan
Copy link

Pauan commented Jun 27, 2019

@asturur Hello, I am speaking on behalf of AmCharts.

We provide a browser-only charting library for our customers. Therefore we do not need node-canvas at all.

We would really like to use Fabric, since it is a wonderful library, but we cannot do so, because of the required dependency on node-canvas.

We are aware that node-canvas downloads prebuilt binaries, but that is still unacceptable for us for the following reasons:

  • The pre-built binaries are only available for common OSes, but some of our customers use exotic OSes.

  • The pre-built binaries are not always up to date.

  • The pre-built binaries can fail for other reasons (misconfiguration, the network being down, etc.)

When any of the above situations happen, the customer gets a huge and scary error message:

gyp ERR! configure error
gyp ERR! stack Error: Can't find Python executable "C:\Users\Foo\AppData\Local\Programs\Python\Python36-32\python.EXE", you can set the PYTHON env variable.
gyp ERR! stack     at PythonFinder.failNoPython (C:\prog\nodejs\node_modules\npm\node_modules\node-gyp\lib\configure.js:483:19)
gyp ERR! stack     at PythonFinder.<anonymous> (C:\prog\nodejs\node_modules\npm\node_modules\node-gyp\lib\configure.js:508:16)
gyp ERR! stack     at C:\prog\nodejs\node_modules\npm\node_modules\graceful-fs\polyfills.js:284:29
gyp ERR! stack     at FSReqWrap.oncomplete (fs.js:152:21)
gyp ERR! System Windows_NT 10.0.10586
gyp ERR! command "C:\\prog\\nodejs\\node.exe" "C:\\prog\\nodejs\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd C:\Users\Foo\AppData\Local\Temp\ggg\fabric.js\node_modules\canvas
gyp ERR! node -v v8.11.2
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules\canvas):
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] install: `node-gyp rebuild`
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: Exit status 1

Even though installation may technically have succeeded, our customers panic because it certainly looks like there is something wrong.

After getting this message, they contact our customer service. It is quite frustrating for both us and our customers that we have to inform them that we cannot fix the issue (because the issue is with node-canvas).

And requiring our customers to go through the installation guides is not acceptable. They pay us money and they expect a very simple works-out-of-the-box experience. For similar reasons, we cannot tell them to just ignore the error.

The canvg library solved this issue by making canvas a peerDependency. That means people using Node can still use canvas, but people who are not using Node can avoid downloading canvas completely.

@asturur
Copy link
Member

asturur commented Jun 29, 2019

Hi @Pauan , thanks for your interest in fabricjs first of all.
I want to answer sincerly, putting all my opinions and ideas about it, i do not aim at being rude, just honest.

We should first consider that removing a dep is a breaking change, so i should wait for a 4.0 release anyway.

Then there is something I' m not really happy about, the tradeoff.
Fabric is obviously more useful in a browser than in node, no doubt about it.
But here we are talking about your paying customers, that are developers and are supposed to not get scared at a log, and a not failing installation. Just a log that contains npm WARN and gyp ERR.
Armchart is a commercial library and I expect it to have better docs than fabricjs, I m sure there is an installation section that may solve that anyway.

Node developers on the other hand from that day on, have to add node-canvas manually.

Making a change to the installation process to favor some devs over others, just because the former do not want to take their time to understand a log, to me feels like the defeat of the whole purpose of this repository that for me has always been learning.

I have also asked myself why there are no options in npm to suppress log per packages or to specify a browser/node installation mode, everything brings to the point of having a different package or to favor a kind of user over another.

now .npmrc has an option called loglevel that can be set to silent, I wonder if that is used when the package is used as a dependency or not. We can try that, the alternative then is to make a script that removes the dependency from the package.json and to run that script each release to publish a different package. This would flatter me anyway.

I have a question tho. armchart is not yet using fabricjs, so i would like to know if this:

Even though installation may technically have succeeded, our customers panic because it certainly looks like there is something wrong.

After getting this message, they contact our customer service. It is quite frustrating for both us and our customers that we have to inform them that we cannot fix the issue (because the issue is with node-canvas).

is a projection of a possible future, or is a test run with an armchart version that supports fabric.

@Pauan
Copy link

Pauan commented Jun 29, 2019

We should first consider that removing a dep is a breaking change, so i should wait for a 4.0 release anyway.

Yup, I know. But if I didn't bring up the problems now, then the change would never happen.

But here we are talking about your paying customers, that are developers and are supposed to not get scared at a log

Our customers have a wide range of abilities. A few of them are quite experienced, but many of them are very inexperienced, which is why they pay us for customer service.

Yes, some of them do get scared by logs, and no, adding more documentation does not help. We have extensive documentation (including for installation), but it does not help, especially because many of our customers do not read the documentation in the first place.

Even for very simple things (which are covered in the documentation), we have to repeatedly answer the same questions over and over again. It's not possible to force people to learn.

I know this can be hard to believe (especially if you're an experienced developer), but it is the reality of our business.

Node developers on the other hand from that day on, have to add node-canvas manually.

Node users merely have to run npm install canvas or yarn add canvas. It's a very simple step. And npm / yarn will warn them if they do not take that step.

Making a change to the installation process to favor some devs over others

Right now the installation process favors Node users, because browser users must always pay the cost of downloading canvas even though they do not need it (including the complexity of installing it).

I have also asked myself why there are no options in npm to suppress log per packages or to specify a browser/node installation mode, everything brings to the point of having a different package or to favor a kind of user over another.

I agree that the fault ultimately lies with npm and their lack of truly optional dependencies (or compilation targets), but since they lack such things, it's up to us to figure out workarounds.

We don't have the luxury of waiting years for npm to create such a system (if they ever do).

I have a question tho. armchart is not yet using fabricjs, so i would like to know if this

We used Fabric in the past, but because of the issues we had with our customers, we switched to canvg.

But since Fabric is much more feature-filled than canvg we would really like to switch back to Fabric.

@finom
Copy link
Contributor Author

finom commented Jun 29, 2019

Guys, I've created a package for these needs: https://www.npmjs.com/package/fabric-pure-browser
It uses Travis CI cron job once a day to pull fabric.js, build it and publish if a new version appears, so the versioning isn't gone. Deployment script makes optionalDependencies to be an empty object so you wouldn't get such issues any more. You can check out how it works here: https://github.com/finom/fabric-browser/blob/master/publish.js

@asturur I've got one idea: why not to publish a browser-only fabricjs version with a "-browser" postfix as I did for "fabric-pure-browser" package?

@asturur
Copy link
Member

asturur commented Jun 30, 2019

would be nice to keep it in the main fabricjs org.
I like the idea of publishing a version that is -browser postfixed!
@finom would you like to take care of the boilerplate setup with a PR?

asturur pushed a commit that referenced this issue Jul 2, 2019
The script temporarily overrides package.json contents with `optionalDependencies: {}` and `version: 'FABRIC_VERSION-browser'`, publishes it, restores the original package.json and publishes the main version. 
It can be run with `node publish` and it also supports original `npm publish` parameters: `node publish --dry-run`. 
The file is eslinted with .eslintrc.json. Closes #5735.
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this issue Nov 18, 2019
The script temporarily overrides package.json contents with `optionalDependencies: {}` and `version: 'FABRIC_VERSION-browser'`, publishes it, restores the original package.json and publishes the main version. 
It can be run with `node publish` and it also supports original `npm publish` parameters: `node publish --dry-run`. 
The file is eslinted with .eslintrc.json. Closes fabricjs#5735.
@gilbertl
Copy link

Hi, I see that "fabric" now has versions postfixed -browser.

I don't think this solves the problem, as yarn add fabric:3.6.3-browser still comes with canvas as an optional dependency.

In the mean time, I'm going to use finom's fabric-pure-browser

@asturur
Copy link
Member

asturur commented Apr 15, 2020

mmm very strange, it should not have the the dependency listed.

@alexandros-megas
Copy link

@finom As someone who is currently diagnosing a broken build, 5:30PM on a Friday, thank you for fabric-pure-browser 🙌

@anselm94
Copy link

@finom As someone who is currently diagnosing a broken build, 5:30PM on a Friday, thank you for fabric-pure-browser 🙌

For anyone to use Typescript typings from fabric (i.e. @types/fabric) into fabric-pure-browser

  1. Create a definition file - typings/fabric-pure-browser/index.d.ts
/// <reference types="fabric" />

declare module 'fabric-pure-browser' {
	export * from 'fabric';
}
  1. Include this in your tsconfig.json, if not already done
{
    ...
    "compilerOptions": {
        ...,
        "typeRoots": ["./typings", "./node_modules/@types"]
    }
}

@asturur
Copy link
Member

asturur commented Apr 11, 2024

consider fabric exists with a browser counterpart in the main repo and has the -browser suffix on the version.

@asturur
Copy link
Member

asturur commented Apr 11, 2024

https://www.npmjs.com/package/fabric/v/5.3.0-browser

@taranveerjohal
Copy link

Hey All,
I was following this conversation and was able to remove the jsdom and canvas dependency from my fabric package using the browser prefix. BUT I have a somehow related issue with typings. I am using React btw.

I need to set limit on caches when rendering heavy SVGs and I am using,

async function fabricLoadFloorSVGFromURL(
  canvas: fabric.Canvas,
  url: string,
  deviceScale: any,
): Promise<{
  svgMap: fabric.Object | fabric.Group;
  imageElementForMap: HTMLCanvasElement;
}> {
  return new Promise((resolve, reject) => {
    try {
      fabric.loadSVGFromURL(url, (objects, options) => {
        // the actual SVG
        const map = fabric.util.groupSVGElements(objects, options);
        debug('Map Image Loaded: 2', map);

        map.objectCaching = true;

        // * These are used to set the limit of the cache size
        fabric.perfLimitSizeTotal = 250000000000;

        //* Pixel limit for cache canvases width or height. IE fixes the maximum at 5000
        fabric.maxCacheSideLimit = 50000;

        //* Lowest pixel limit for cache canvases, set at 256PX
        fabric.minCacheSideLimit = 4000;

        // the copied Image Element which is needed to improve the quality of the image on drag and zoom
        // This will be swapped with the actual SVG when performing the drag and zoom
        const imageElementForMap = map.toCanvasElement({
          format: 'png',
          enableRetinaScaling: true,
          withoutTransform: true,
          withoutShadow: true,
        });

        canvas.setBackgroundImage(
          map as fabric.Image,
          // imageElementForMap.toDataURL(),
          canvas.renderAll.bind(canvas),
          {
            left: map.width! / 2,
            top: map.height! / 2,
            originX: 'center',
            originY: 'center',
          },
        );

        canvas.setBackgroundColor('white', canvas.renderAll.bind(canvas));

        // Set the device size here
        const zoomFactor = Math.min(
          canvas.getWidth() / map.getScaledWidth()!,
          canvas.getHeight() / map.getScaledHeight()!,
        );

        deviceScale.current =
          (Math.min(map.height!, map.width!) * deviceScale.current) /
          STANDARD_DEVICE_SIZE; // Set the scale of the devices

        canvas.setZoom(zoomFactor);

        const leftOffset = (map.width! * zoomFactor - canvas.width!) / 2;
        const topOffset = (map.height! * zoomFactor - canvas.height!) / 2;
        canvas.relativePan({ x: -leftOffset, y: -topOffset });
        resolve({ svgMap: map, imageElementForMap });
      });
    } catch (error) {
      debug('Error in fabricLoadFloorSVGFromURL', error);
      reject(error);
    }
  });
}

The issue i have is, the properties perfLimitSizeTotal , maxCacheSideLimit, minCacheSideLimit seems to be not existing in the typing that are coming from @types/fabric.

I am getting the error

Property 'perfLimitSizeTotal' does not exist on type 'typeof import("/app/node_modules/@types/fabric/fabric-impl")

and I am unable to set the properties. I am using

 "fabric": "5.3.0-browser",
 "@types/fabric": "^5.3.7",

Please assist me in setting the values.

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 a pull request may close this issue.

8 participants