fabricjs / fabric.js

Javascript Canvas Library, SVG-to-Canvas (& canvas-to-SVG) Parser
http://fabricjs.com
Other
28.7k stars 3.49k forks source link

Get rid of node-canvas dependency #5735

Closed finom closed 5 years ago

finom commented 5 years ago

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 commented 5 years ago

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.

finom commented 5 years ago

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

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

Pauan commented 5 years ago

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

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: fsevents@1.2.4 (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: canvas@1.6.13 (node_modules\canvas):
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: canvas@1.6.13 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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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?

gilbertl commented 4 years ago

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 commented 4 years ago

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

alexandros-megas commented 4 years ago

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

anselm94 commented 5 months ago

@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'; }

2. Include this in your `tsconfig.json`, if not already done
```jsonc
{
    ...
    "compilerOptions": {
        ...,
        "typeRoots": ["./typings", "./node_modules/@types"]
    }
}
asturur commented 5 months ago

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

asturur commented 5 months ago

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

taranveerjohal commented 2 months ago

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.