Fabric.js: Critical dependency: the request of a dependency is an expression

[pipriles]

This is not a bug, it's a warning that happens when you import fabric.js with webpack.
I managed to remove the warning changing where fabric.canvasModule is used to 'canvas'.

Version

2.0.0-beta5

Steps to reproduce

Import fabric with webpack

Expected Behavior

No warnings

Actual Behavior

It gives this warning:

WARNING in ./node_modules/fabric/dist/fabric.js
27411:14-42 Critical dependency: the request of a dependency is an expression

[asturur]

ok. note taken.
Really do not know what to do about it since being able to switch bewteen canvas and canvas-prebuilt is a feature more usefull than not get a warning.

I belive that webpack warns you because of course it does not know what to pack if that value change at runtime.
Is also true that that require is for node only where webpack is not needed.

Probably a warning we will keep.

[pipriles]

Uhm i see... Maybe making two different files. But i don't like that idea...

[asturur]

To be honest fabricjs do not play well with webpack.
First of all it triest to pull in jsdom because it thinks to be under node, and that is wrong.
Second it should not require canvas at all or canvas-prebuilt since if you are using webpack you are probably targeting a browser.

This is what we should fix.

make fabric-js webpack friendly.

I accept suggestions.

[asturur]

@stefanhayden do you have any idea about detecting a webpack build from inside a library and deactivating some imports?

[jparkrr]

Second it should not require canvas at all or canvas-prebuilt since if you are using webpack you are probably targeting a browser.

I use webpack. Just wanted to let you know that I need canvas for tests, so you'd also have to special case that.

Right now I'm using a personal fork to get around this warning.

Not sure what the correct solution is, but I think it doesn't seem crazy to publish a fabric-webpack, which is a custom build with the dynamic requires replaced.

[stefanhayden]

@asturur I was talking about this with @itsjoesullivan the other day. I think we could write a webpack plugin that uses eslint style comments to delete lines of code based on options passes in to webpack. I think it might be something I explore soon.

[asturur]

i would be ok with setting canvas as a dev dep in my project if i need it for tests, or maybe as a peerDependency for anyone.

whatever fits with current fabricjs tests system is ok.

the point in the specific case of webpack is that i would not include canvas in the packing process at all if it would be possible.

[asturur]

@stefanhayden oh right the new webpack has this comments thing to name chunks.

[stefanhayden]

@asturur in this case it's not the naming of chunks but the whole sale deletion of code upon build.

[asturur]

i understood. i was just sayin that this comment reading is a webpack 3 new thing

[nm]

Any news on this one? I am also getting this warning and it would be great to have a webpack friendly version of fabricjs.

[asturur]

so there are 3 things to make friendly:

custom build in npm if ever possible
peerDeps to avoid installing jsdom and canvas
requiring a constant for require and not a var.

peerDeps would solve npm install issues and would avoid webpack to try to bundle jsdom.

is this warning a warning or it halts the installation? a warning may be fine of you know what are you doing.

[sconix]

We are also experiencing this and even though its a warning it would be really nice to get rid of the warning. We also have another issue with the packaging, the canvas dependency is causing also problems for a firebase cloud functions deployment. For that to work the canvas-prebuilt (2.0.0-alpha.x) version is needed for this package to work in the cloud functions environment.

So perhaps there could be browser-only and prebuilt-canvas package versions as well? I understand if you would like to find a single package solution, but not sure if such exists so that it works for all use cases. Anyway would be nice to be able to use official package instead of building own version which we do now.

[asturur]

ok so i made this thing here:

https://travis-ci.org/fabricjs/fabricjs-webpack/

this is just an example, i took latest webpack, latest node ( to have latest npm ) and build a simple webpack bundle with fabricJS.

What i can see is that the build succedes, there are information stating that xmldom and jsdom are not bundled.

Then i test this on travis with a linux docker image where canvas can't be installed because libs are missing, npm throws warning says that an unmet optional dep is missing ( canvas ) but the intall does not fail. The build run.

i do not see warnings for this problem anymore, is something improve with latest webpack maybe?

[asturur]

since we delegated completely to jsdom and we do not require canvas explicitly anymore, we should be closing this.