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

Not working with browserify #129

Closed
ryanzec opened this issue Nov 20, 2014 · 27 comments
Closed

Not working with browserify #129

ryanzec opened this issue Nov 20, 2014 · 27 comments
Labels

Comments

@ryanzec
Copy link

ryanzec commented Nov 20, 2014

While browserify compiles this library fine, when I try to use it, it get this in the console:

Uncaught Error: Cannot find module '../src'

and when I look at the browserify generated code, I see stuff like this:

},{"../src":undefined,"./utils":174}],169:[function(require,module,exports){

Why is this not working (using version 0.1.15)?

@ryanzec
Copy link
Author

ryanzec commented Nov 20, 2014

I think this has to do with the fact that you can't include the main file defined in the package.json file in your code with browserify. I created a duplicate of src/index.js and called it src/self.js and then replace all instances of require('../src') with require('./self') and the browserify compiled code works fine. I first tried to replace all instances of require('../src') with require('./index') however that would just produce the same error but say ./index could not be found instead of ../src.

Unless I am missing something with browserify (I am relatively new to it), I think this is the issue, I can create a PR to fix this is a better way if this in fact is a problem with this code and not browserify or how I am using browsetify, just let me know.

@spoike
Copy link
Member

spoike commented Nov 20, 2014

This is weird. I cannot reproduce this since it is working in a browserify build I use at work.

Are you using the npm package or the bower one?

@jcampalo
Copy link

I'm using the bower package, maybe the problem it's that browserify looks into the npm instead of bower?

@donspaulding
Copy link

I don't think it's specific to Bower, as I'm seeing this exact error with the npm package as well.

@donspaulding
Copy link

In my particular case, I'm using gulp and the browserify api to split my JS across two bundles, one for vendor scripts (which includes Reflux) and one for my app scripts (which has a var Reflux = require('reflux'); that resolves to the version bundled in vendor.js).

For reference, here's my gulpfile:

'use strict';
var gulp        = require('gulp');

var $           = require('gulp-load-plugins')();
var source      = require('vinyl-source-stream');
var browserify  = require('browserify');
var es6ify = require('es6ify');
var runSequence = require('run-sequence');


var dev = true;

var EXTERNALS = Object.keys(require('./package.json').dependencies);

gulp.task('vendor', function() {
    var bundler = browserify({
        extensions: ['.jsx'],
        debug: dev,
    });
    EXTERNALS.forEach(
        function(x){
            bundler.require(x);
        }
    );
    bundler.add(es6ify.runtime)
    .transform('reactify', {es6: true});

  return bundler.bundle()
    .pipe(source('vendor.js'))
    .pipe(gulp.dest('.tmp/scripts'))
  ;
});

gulp.task('scripts', function() {
    var bundler = browserify('./app/scripts/app.js', {
        extensions: ['.jsx'],
        debug: dev,
        bundleExternal: false,
    });
    EXTERNALS.forEach(function(x){ bundler.exclude(x).external(x);});

    bundler.add(es6ify.runtime)
    .transform('reactify', {es6: true});

  return bundler.bundle()
    .pipe(source('app.js'))
    .pipe(gulp.dest('.tmp/scripts'))
  ;
});


gulp.task('compass', function() {
  return gulp.src('app/styles/*.scss')
    .pipe($.plumber())
    .pipe($.compass({
      css: '.tmp/styles',
      sass: 'app/styles'
    }));
});

gulp.task('imagemin', function() {
  return gulp.src('app/images/*')
    .pipe($.imagemin({
            progressive: true,
            svgoPlugins: [{removeViewBox: false}]
    }))
    .pipe(gulp.dest('dist/images'));
});

gulp.task('copy', function() {
  return gulp.src(['app/*.txt', 'app/*.ico'])
    .pipe(gulp.dest('dist'));
});

gulp.task('bundle', function () {
  var assets = $.useref.assets({searchPath: '{.tmp,app}'});
  var jsFilter = $.filter(['**/*.js']);
  var cssFilter = $.filter(['**/*.css']);
  var htmlFilter = $.filter(['*.html']);

  return gulp.src('app/*.html')
    .pipe(assets)
    .pipe(assets.restore())
    .pipe($.useref())
    .pipe(jsFilter)
    .pipe($.uglify())
    .pipe(jsFilter.restore())
    .pipe(cssFilter)
    .pipe($.autoprefixer({
      browsers: ['last 5 versions']
    }))
    .pipe($.minifyCss())
    .pipe(cssFilter.restore())
    .pipe(htmlFilter)
    .pipe($.htmlmin({collapseWhitespace: true}))
    .pipe(htmlFilter.restore())
    .pipe($.revAll({ ignore: [/^\/favicon.ico$/g, '.html'] }))
    .pipe($.revReplace())
    .pipe(gulp.dest('dist'))
    .pipe($.size());
});

var livereload = true;
var openNewTab = true;

gulp.task('webserver', function() {
  return gulp.src(['.tmp', 'app'])
    .pipe($.webserver({
      livereload: livereload,
      open: openNewTab
    }));
});

gulp.task('serve', function() {
  runSequence(['vendor', 'scripts', 'compass'], 'webserver');

  gulp.watch('app/*.html');

  gulp.watch('app/vendor.js', ['vendor']);

  gulp.watch(['app/scripts/**/*.js', 'app/scripts/**/*.jsx'], ['scripts']);

  gulp.watch('app/styles/**/*.scss', ['compass']);
});


gulp.task('build', function() {
  dev = false;

  runSequence(['scripts', 'compass', 'imagemin', 'copy'],
              'bundle');
});

@donspaulding
Copy link

Also, if I build a single app.js bundle that includes all my dependencies, this issue seems to go away. In my gulpfile above, that can be tested by commenting out the bundleExternal: false line and the EXTERNALS.forEach loop in the scripts task. Those changes are sufficient to include reflux into the app.js and then relative require of ../src/ seems to work again.

@ryanzec
Copy link
Author

ryanzec commented Nov 20, 2014

I am using the npm version also compiling 2 files (a libraries.js and application.js).

@donspaulding
Copy link

So, after making the changes @ryanzec mentioned in his first comment on a fork, I get past the errors with Refluxjs, but immediately have problems with a require('react/addons'). Perhaps that points to browserify being the problem?

@ryanzec
Copy link
Author

ryanzec commented Nov 20, 2014

@donspaulding I am also using react/addons however I don't run into any issue with it after fixing reflux to compile properly with browserify.

@spoike
Copy link
Member

spoike commented Nov 20, 2014

I tried setting up browserify build with libraries as seperate build, but never got it to work with several libraries I was using which wasn't just reflux. So I was figuring it was a browserify problem.

@ryanzec
Copy link
Author

ryanzec commented Nov 20, 2014

Since this issue is not something I am only seeing, should I create a PR that will at least fix the issue to get Rerflux working properly?

If not I will just create a fork that I will personally use that contains code that will work (otherwise I would have to pick another flux based option).

@donspaulding
Copy link

@spoike: yeah, it's not at all straightforward to get this working, at least with gulpjs + browserify

I think this problem is related to browserify/browserify#933 and has been "solved" by browserify by forcing people to use -r ./path/to/module.js instead of -r ./path/to/module.

@ryanzec
Copy link
Author

ryanzec commented Nov 20, 2014

well using this -r ./node_modules/reflux/src/index.js:reflux does not work for me

@ryanzec
Copy link
Author

ryanzec commented Nov 20, 2014

Ironically the code that does work with browserify:

https://www.github.com/ryanzec/refluxjs.git#1d881fe15618967f5ec856fc4e597ef23adc8732

does not run the mocha tests properly (though the code does seems to work for me).

@ryanzec
Copy link
Author

ryanzec commented Nov 23, 2014

For whatever reason, this is not happening anymore for the latest code. Not sure if the changes 2 days ago fixed this or if it was another library I was using that was causing the conflict (I and since replaced some other libraries I would working with).

Guess I will close this issue.

@ryanzec ryanzec closed this as completed Nov 23, 2014
@ryanzec
Copy link
Author

ryanzec commented Nov 23, 2014

actually it is still an issue

@ryanzec ryanzec reopened this Nov 23, 2014
@ryanzec
Copy link
Author

ryanzec commented Nov 23, 2014

So my version was working fine until I tried to run tests through Mocha when it started to fail in weird ways.

Doing more investigation, if I set the main as dist/reflux.js for reflux's package.json, browserify and running tests through mocha both work nicely.

Another thing I will mention which is I am running node v0.11.14 (because I am also running KoaJS which requires generators). I don't think this is an issue though since I did try to build with v0.10.33 and got the same error from the browserified file.

@ryanzec
Copy link
Author

ryanzec commented Nov 23, 2014

I think the issue is related to the issue that browserify does not work well with circular dependencies. For example src/index.js requires src/createAction.js however src/createAction.js requires src/index.js (by requiring ../src). I can get the compiled code to work in the browser by removing the circular dependency however a few tests fail and not sure how to get them to work, don't understand the code that well.

This kinds of makes sense on why when have a copy of index that the other files require makes the code work in the browser and why tests fail.

Maybe using the standalone options (which I can't did in my case) makes the dist/reflux.js file work.

@ryanzec
Copy link
Author

ryanzec commented Nov 23, 2014

Ok, spent a bit of the day trying to get this to work and I think I finally got it. I have refactored the files/functions that are causing circular dependencies that seems to be causing issue for browserify. I don't know that specifics of the issue, just read that it can be one (if I find time, I might try to read up on it). I have created PR #138 that should resolve these issue (using this branch as my reflux module in my package.json resolves the issuse I have reported here).

Please let me know if there is anything else I need to do to this PR to get is merged. Not being able to use this module through npm is a pretty big blocker and I really prefer the simplicity of this implementation of Flux vs Facebook's more complex implementation.

@spoike spoike added the bug label Dec 7, 2014
@willembult
Copy link
Contributor

I had a separate lib-build working fine by putting this in package.json:

"browser": {
  "reflux": "./node_modules/reflux/dist/reflux.js"
}

However, in the latest npm package, the dist folder went missing so this now breaks. Was that intentional or did it accidentally not get included in the package?

@spoike
Copy link
Member

spoike commented Dec 12, 2014

@willembult well... removing uneccesary files was requested in #125

@willembult
Copy link
Contributor

Interesting... that's probably actually the way it should be, the file just allowed to work around the real issue that's described here. Using bower package now to do the same.

@mschipperheyn
Copy link

Is there a quick workaround for this? Just spent a few hours trying to find out why my gulp file suddenly stopped running and it turns out it's this.

@spoike
Copy link
Member

spoike commented Dec 31, 2014

@mschipperheyn: quickest workaround is to use the built file instead, which is available on bower

@mschipperheyn
Copy link

This seems to work for me to prevent gulp from choking. Although it still requires me to reference a bower reflux file since reflux is no longer built. Hopefully this bug will be resolved soon. This was my first experience w reflux. It's not really a great way to get started with it.

var config = {
    dependencies: [
    'react',,
    'react/addons',
    'react-router',
    //'reflux'
    ],
    BUILD:'./build',
    DIST:'./dist',
    development:true
};

gulp.task('build-vendor', function(){

    var vendorsBundler = browserify({
      debug: true, 
      require: config.dependencies,
      exclude:[
        'reflux'
      ]
    });

    vendorsBundler.bundle()
      .on('error', gutil.log)
      .pipe(source('scripts/vendors/vendors.js'))
      .pipe(gulpif(!config.development, streamify(uglify())))
      .pipe(gulp.dest(config.BUILD)); 

});

gulp.task('build-app', function(){
    var appBundler = browserify({
        entries: ['./src/scripts/app.jsx'], // The entry file, normally "main.js"
        transform: [reactify],
        debug: config.development,
        extensions: ['.jsx'],
        cache: {}, packageCache: {}, fullPaths: true
    });

    (config.development ? config.dependencies : []).forEach(function (dep) {
        appBundler.external(dep);
    });
        appBundler.external('reflux');
    appBundler.bundle()
      .pipe(source('scripts/app.jsx'))
      .pipe(rename('scripts/app.js'))
      .pipe(gulpif(!config.development, streamify(uglify())))
      .pipe(gulp.dest(config.BUILD))
      .pipe(gulpif(config.development, livereload()));
});

@mschipperheyn
Copy link

One last thing I noticed here is that the require('Reflux') dependency is case sensitive. Just for people working their way through this stuff.

Please fix this issue asap. I don't think you appreciate just how sucky this issue really is.

@spoike spoike closed this as completed in e498c99 Jan 3, 2015
@spoike
Copy link
Member

spoike commented Jan 3, 2015

Actually it worked to include reflux if you referenced it by relative path to the installed npm package's main file node_modules/reflux/src/index.js like this:

var gulp = require('gulp')
    browserify = require('browserify'),
    source = require("vinyl-source-stream");

gulp.task('scripts-vendor', function() {
    return browserify()
        .require("./node_modules/reflux/src/index.js", { expose: 'reflux'})
        .bundle()
        .pipe(source("vendor.js"))
        .pipe(gulp.dest("./dist"));
});

gulp.task('scripts-app', function() {
    return browserify("./application.js")
        .external("reflux")
        .bundle()
        .pipe(source("app.js"))
        .pipe(gulp.dest("./dist"));
});

gulp.task('default', ['scripts-vendor', 'scripts-app']);

Fixed the issue on my side and it should work by doing .require("reflux") instead now, as many have lamented over. I.e. for the code example above:

gulp.task('scripts-vendor', function() {
    return browserify()
        .require("./node_modules/reflux/src/index.js", { expose: 'reflux'})
        .bundle()
        .pipe(source("vendor.js"))
        .pipe(gulp.dest("./dist"));
});

Sorry for the time it took to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants