hughsk / disc

:chart_with_upwards_trend: Visualise the module tree of browserify project bundles and track down bloat.
http://hughsk.io/disc
Other
1.33k stars 83 forks source link

TypeError: Cannot read property 'source' of undefined #30

Open cowwoc opened 9 years ago

cowwoc commented 9 years ago

This issue is a variation of https://github.com/hughsk/disc/issues/24 (same error but I use absolute paths).

  1. Run the gulp code found at https://github.com/hughsk/disc/issues/29#issuecomment-71417368 but replace the paths (passed into add() and dest()) with absolute paths.
  2. Exception is thrown:
C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\index.js:99
        size: row.source.length
                 ^
TypeError: Cannot read property 'source' of undefined
    at tree.name (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\index.js:99:18)
    at flat.unflatten.delimiter (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\node_modules\file-tree\index.js:15:5)
    at next (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\node_modules\file-tree\node_modules\async-reduce\index.js:11:7)
    at reduce (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\node_modules\file-tree\node_modules\async-reduce\index.js:23:3)
    at tree (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\node_modules\file-tree\index.js:13:3)
    at json (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\index.js:95:3)
    at bundle (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\index.js:129:10)
    at BufferList._callback (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\index.js:27:5)
    at BufferList.end (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\node_modules\bl\bl.js:75:10)
    at Stream.method [as end] (C:\Users\Gili\Documents\realestate\frontend\node_modules\disc\node_modules\duplexer\index.js:47:39)

This issue is reproducible 100% of the time.

hughsk commented 9 years ago

@cowwoc could you please include the bundled output from browserify that's breaking here? I need to be able to reproduce it on my machine to be able to help with this one.

cowwoc commented 9 years ago

@hughsk Please email me at [scrubbed] and I'll send you the bundle privately.

GalCohen commented 9 years ago

got the same issue here too

chpio commented 9 years ago

here too :(

formula1 commented 9 years ago

If you are on windows, you will likely need to do a path.normalize(__dirname+"/path/to/file) when creating your bundle.

It seems as though browserify doesn't do it for you : /

A working Example (for me):

function(req,res){
  browserify({
    fullPaths: true
  })
  .add(path.normalize(__dirname+"/browser.js"))
  .ignore("prompt")
  .transform({
    global: true
  }, 'uglifyify')
  .bundle().pipe(disc()).pipe(res);
}

Full code here. Go to localhost:3000/fresh or localhost:3000/disk

An alternative is require.normalize which allows you to give a valid require-able and it will return a valid string that matches your operating systems pathing structure (ie windows or everone else)

Btw, if theres ever an issue with file loading and you're on windows:

nemo commented 9 years ago

This still happens to me even after using fullPaths: true, but only when I generate the bundle through gulp. Using command line works just fine.

Here's the gulp task:

var gulp = require('gulp');
var config   = require('../config');
var browserify = require('browserify');
var open = require('opener');
var disc = require('disc');
var fs = require('fs');

gulp.task('build:disc', function() {
    var output = config.destDirectory + '/disc.html';

    var bundler = browserify(config.sourceDirectory + '/' + config.sourceEntry, {
      fullPaths: true
    });

    bundler.bundle()
      .pipe(disc())
      .pipe(fs.createWriteStream(output))
      .once('close', function() {
        open(output);
      });

});
formula1 commented 9 years ago

@nemo are you on windows?

nemo commented 9 years ago

@formula1 Nope - OSX.

formula1 commented 9 years ago

What is the value of config.souceDirectory + "/" config.sourceEntry? I've been able to get this running on multiple systems so I was hoping it was a windows only issue

nemo commented 9 years ago

Here's how config is defined:

var config = {};

config.destDirectory = "./dist";
config.sourceDirectory = "./app";
config.sourceEntry = 'index.js';

And in case it's helpful, here's the versions for what I'm using:

{
    "browserify": "^9.0.7",
    "disc": "^1.3.2",
    "gulp": "^3.8.11",
    "opener": "^1.4.1"
}
formula1 commented 9 years ago

./Anything is a relative path similar to ../something/above. So when you specify the {fullPaths:true} What your implicitly saying is here is a direct path from point A to point B without taking advantage of the cwd. so instead of ./dist you'll want __dirname+"/dist" This will result in something like /home/name/something/else/path/to/your/file/dist.

I'm sorry if that is confusing, not sure the best way to explain.

cheton commented 9 years ago

In index.js#L91, you can resolve mod.id to an absolute path if it is a relative path. For example:

  var byid = modules.reduce(function(memo, mod) {
    var id = (function(id) {
      if (path.resolve(id) !== path.normalize(id)) { // check for relative path
        return path.resolve(process.cwd(), id); // resolves "id" to an absolute path
      }
      return id;
    }(mod.id));
    memo[id] = mod;
    return memo
  }, {})

I'm not sure if above code example may cause any side effects, but it did fix my issue even if using a relative path.

nemo commented 9 years ago

Ah I see. Yeah, using __dirname and path.normalize fixed the problem for me :)

formula1 commented 9 years ago

You probably don't need normslize btw, that's usually a windows issue. Mac and Linux can generally path as freely as they like so long as the code base isn't going to be shared to windows

nemo commented 9 years ago

Without path.normalize, the error kept showing up on OS X because I had relative paths based on the __dirname in there. (__dirname + "/../../index.js" etc.)

formula1 commented 9 years ago

Ah, makes sense.

JorritPosthuma commented 9 years ago

With @cheton his patch, it also fixed it in our setup!

glortho commented 9 years ago

@cheton patch worked for me too