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

Further information needed in the README Installation section #66

Open
iamtmrobinson opened this issue May 30, 2018 · 2 comments
Open

Comments

@iamtmrobinson
Copy link

The installation section currently says to use npm install --save circos with no further instruction.

Doing just this and then using Browserify to require circos results in the following error:
Error: Cannot find module '!!../../node_modules/css-loader/index.js!./tooltip.css' from '/.../node_modules/circos/dist'

Is there an assumption that Webpack is being used by those who try to import circos from npm?
Regardless, it would be useful for the documentation to be fleshed out to cover the different ways of running the Circos package.

@vorg
Copy link

vorg commented Feb 15, 2019

I've just hit the same problem when trying to use circos with budo/browserify...

@amirkdv
Copy link

amirkdv commented Apr 14, 2019

This issue really needs to be addressed since circos.js (as of current version 2.1.0) is unusable with browserify.

The Issue

As @iamtmrobinson points out circos.js does assume that it's being used under webpack. But the place where this assumption is breaking browserify is this line in circos.es6.js with a webpack-specific require.

It is very clear that this line is there for hot module replacement for development purposes and is hidden from execution by being inside an if (false) block which I assume circos.js devs turn to true in their dev environment. This is problematic with browserify because (from the website):

Browserify parses the AST for require() calls to traverse the entire dependency graph of your project.

which means it will see any require anywhere in code even if it's unreachable.

Solution

I'm not too familiar with webpack but I think the cleanest solution is for Circos to use __webpack_require__ or some other function for this webpack-specific line. Even doing something like this will work too:

var someOtherNameForRequire = require;
someOtherNameForRequire('!!../../node_modules/css-loader/index.js!./tooltip.css');

While that hasn't happened, people who need to make circos.js work with browserify need to patch the official release. It would be nice if there was a browserify ignore tag that we could add above that line which does not exist (see discussion of this here). For the time being we can just comment out the entire if (false) block. Here is my patch, applicable via patch -p1 < circos.patch:

diff --git a/node_modules/circos/dist/circos.es6.js b/node_modules/circos/dist/circos.es6.patched.js
index 67460f6..7e7e7b1 100644
--- a/node_modules/circos/dist/circos.es6.js
+++ b/node_modules/circos/dist/circos.es6.patched.js
@@ -18454,6 +18454,10 @@ if(typeof content === 'string') content = [[module.i, content, '']];
 var update = __webpack_require__(457)(content, {});
 if(content.locals) module.exports = content.locals;
 // Hot Module Replacement
+/*
+ * This block is unreachable and used for development purposes only; comment it
+ * out so the `require` invocation does not confuse browserify
+ */ /*
 if(false) {
        // When the styles change, update the <style> tags
        if(!content.locals) {
@@ -18466,6 +18470,7 @@ if(false) {
        // When the module is disposed, remove the <style> tags
        module.hot.dispose(function() { update(); });
 }
+*/

 /***/ }),
 /* 455 */

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

3 participants