jsdf / browserify-incremental

incremental rebuild for browserify
175 stars 13 forks source link

browserifyinc generates different output than the browserify counterpart #3

Closed gabrielecirulli closed 9 years ago

gabrielecirulli commented 9 years ago

I have been trying to get browserify-incremental to work in browserify-rails by making it switch between the browserify and browserifyinc based on an option.

With my current configuration, this command runs:

/myapp/node_modules/.bin/browserifyinc -d --extension=.jsx --fast --cachefile="/myapp/tmp/browserify-rails/browserifyinc-cache.json" -o "/myapp/tmp/browserify-rails/output20150106-13751-ay9tra" -

Where --extension=.jsx and --fast are added by my Rails application's config. The main JavaScript file comes from stdin, and the command is run in the correct directory so that all requires are tracked properly.

This is the corresponding browserify command, when the switch for browserifyinc is disabled:

/myapp/node_modules/.bin/browserify -d --extension=.jsx --fast -o "/myapp/tmp/browserify-rails/output20150106-14638-165eoyg" -

Everything seems the same, however the output of browserifyinc fails with an error when it runs in my browser:

Uncaught ReferenceError: global is not defined (modernizr.js:4)

The error seems weird, it comes from a part of the code that seems completely unrelated (I use modernizr with browserify-shim).

However, when inspecting the output of the two commands with a piece of code as simple as console.log("Test"), I get radically different outputs:

When run with browserifyinc (the above command plus < testfile.js), I get:

(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({"/Users/gabriele/Work/Impraise/impraise-web/app/assets/javascripts/_stream_0.js":[function(require,module,exports){
console.log("Test");

},{}]},{},["/Users/gabriele/Work/Impraise/impraise-web/app/assets/javascripts/_stream_0.js"])

Running the above browserify command, though, I get a radically different and longer output (it seems to contain Buffer, due to the --fast option, but I suspect the differences don't stop there): Gist of both files.

Lastly, the browserify node_module that is installed in my Rails app which browserify-rails uses is version 6.3.4. browserify-incremental depends on ^6.1.0, and going into its node_modules I don't see a browserify directory, so it seems to be relying on the same one the application itself uses if I'm not mistaken.

Could you help me shed some light on this issue? I will be experimenting more options, but if you know anything I think it would help me a lot.


Although I tried removing the file and it didn't make any difference on browserify's output, here are the contents of my package.json. The only thing that might have some bearing (since the error I mentioned above seems to have some correlation with browserify-shim) is the "browserify" key, which specifies 6to5ify and browserify-shim as transforms. Maybe they are not being picked up.

{
  "devDependencies": {
    "6to5ify": "*",
    "browserify": "~> 6.3",
    "browserify-incremental": "^1.0.0",
    "browserify-shim": "^3.8.0",
    "eventemitter2": "^0.4.14",
    "fuse.js": "~> 1.2",
    "humps": "https://codeload.github.com/impraise/humps/tar.gz/master",
    "jquery": "~> 2.1",
    "lodash": ">= 2.4.1",
    "moment": "^2.8.4",
    "react": "^0.12.1",
    "superagent": "~> 0.21",
    "underscore.string": "^2.4.0"
  },
  "license": "MIT",
  "engines": {
    "node": ">= 0.11"
  },
  "browserify": {
    "transform": [
      "6to5ify",
      "browserify-shim"
    ]
  },
  "browserify-shim": {
    "stripe": "global:Stripe",
    "__mixpanel": "global:mixpanel",
    "__youtube": "global:YT",
    "jquery-ujs": {
      "depends": [
        "jquery:jQuery"
      ]
    },
    "jquery.payment": {
      "depends": [
        "jquery:jQuery"
      ]
    },
    "modernizr": {
      "exports": "Modernizr"
    },
    "uservoice": {
      "exports": "UserVoice"
    }
  },
  "browser": {
    "jquery-ujs": "./bower_components/jquery-ujs/src/rails.js",
    "jquery.payment": "./bower_components/jquery.payment/lib/jquery.payment.js",
    "modernizr": "./vendor/assets/javascripts/modernizr.js",
    "uservoice": "./vendor/assets/javascripts/uservoice.js"
  },
  "dependencies": {
    "async": "^0.9.0",
    "cookies-js": "^1.1.0",
    "pluralize": "^1.1.0"
  }
gabrielecirulli commented 9 years ago

I think I have a better inkling of what might be happening (still no idea how to solve it though):

I beautified the generated code and if you look at the part where the error I mentioned before happens, you see this:

{

    // Rest of the bundle

    "/myapp/vendor/assets/javascripts/modernizr.js": [function(require, module, exports) {;
        __browserify_shim_require__ = require;
        (function browserifyShim(module, exports, require, define, browserify_shim__define__module__export__) {
          "use strict";

          /* Modernizr 2.8.3 (Custom Build) | MIT & BSD
           * Build: http://modernizr.com/download/#-svg-cssclasses-addtest-prefixed-teststyles-testprop-testallprops-hasevent-prefixes-domprefixes
           */
          ;
          window.Modernizr = function(a, b, c) {
            // Modernizr code
          }(this, this.document);

          ;
          browserify_shim__define__module__export__(typeof Modernizr != "undefined" ? Modernizr : window.Modernizr);

        }).call(global, undefined, undefined, undefined, undefined, function defineExport(ex) {
          module.exports = ex;
        });

    }, {}],

    // Rest of the bundle

}

The point where the error happens is that line near the bottom where you see }).call(global, undefined, undefined, undefined, undefined, function defineExport(ex) {. If you look closely, you'll notice that it calls the "shimming" function with global as this, yet global doesn't exist.

If you look near the top, in the line that goes "/myapp/vendor/assets/javascripts/modernizr.js": [function(require, module, exports) {;, that's where global should come into play: It should be passed to this file as one of the arguments;

Looking at the output of running browserify instead of browserify-inc, the signature of the same function looks like this: (function(process, global, Buffer, __argument0, __argument1, __argument2, __argument3, __filename, __dirname) {;. As you can see, that takes into account the requirement of global and it does pass it in as an argument. The whole thing works as a result.

This doesn't take me any closer to figuring out why global is not being passed in, but I thought I would ask you for help on this since you might have a better clue than me :smile:

jsdf commented 9 years ago

I'll try to reproduce it, but can you give me the output of npm ls in the root of your app so I can see the exact versions of all modules which are installed?

jsdf commented 9 years ago

Nevermind, it was a bug in my code, I've pushed a new version to npm which should hopefully resolve the issue.

gabrielecirulli commented 9 years ago

Thanks for the fix, the JavaScript now runs in the browser. I'm now encountering another problem with the compilation step: it runs quite fast, but it doesn't seem to detect that files have changed.

In my structure I have an application.js file which is the main file and it requires an (extensive) tree of files that form the full application. What I'm experiencing right now is that when I change one of the files down in the tree, the change is not picked up and after the code is bundled, I still get the old behavior. The only way I could get things to update was to edit application.js, and changes there would be reflected immediately.

Is this a known "gotcha" in browserify-incremental or might there be something wrong I'm doing? I can try to help you by reproducing an environment where this happens, though I won't be able to do that very soon.

As a way to maybe troubleshoot this, right now browserifyinc outputs to a temp file (which gets deleted right after it's done), but the cache is stored in a Rails directory and persists between runs. Does the output file need to remain present too? I can make it persist as well, if necessary.

jsdf commented 9 years ago

I don't think the temp file is needed, only the cache file (which also stores modified-times of all dependency files). browserify-incremental should be able to detect any changed files, as long as their mtime has increased (which it should if you are changing and saving a file normally).

I've been adding some proper tests to this project but so far I have not been able to reproduce the issue you're describing.

jsdf commented 9 years ago

Bear in mind that some filesystems have a minimum resolution of 1-2 seconds for file mtimes, so any changes occuring more frequently than that might not be detected.

gabrielecirulli commented 9 years ago

Thanks for your answer. I think it's probably not due to the mtime resolution thing since I think I wasn't making changes at that rate. Regarding the output file, I included it because I thought it was mandatory based on the description in the readme file. The way Sprockets works (the Ruby on Rails gem I'm trying to integrate it with) is that it requires code to be just streamed through it, and it doesn't expect you to create new files. Since browserify-incremental doesn't output to stdout, I simply put the output somewhere temporary, read it, pipe it into Sprockets and just get rid of it afterwards.

When I have time, I will see if I can come up with a minimal test case where the issue happens. Thanks for the help so far, I really appreciate it and it's gonna give me a major speed boost in my workflow if we get it working 😃

jsdf commented 9 years ago

If it helps, the latest version of browserify-incremental now supports output to the stdout (if you omit the -o arg).

gabrielecirulli commented 9 years ago

That's great to hear, it lets me simplify a lot! Thanks!

gabrielecirulli commented 9 years ago

Okay, I tried again today and maybe the issue wasn't there after all. Apparently, my Rails app had somehow been tracking an old version of https://github.com/impraise/browserify-rails. After I updated, dependencies seem to be detected correctly (because I fixed a bug where the library was using browserifyinc to also get the --list results, but that doesn't work right now).

This shaved a good 9 seconds off of my requests. That's already a good improvement compared to the almost 20 seconds it took for the JS to be bundled before. I will merge this into Impraise and try doing actual development with it. I'll let you know if anything seems strange.

The only real bottleneck now is the --list call. On every request, browserify-rails runs browserify twice, passing the --list argument the first time. I had to make my custom version of browserify-rails always use the vanilla browserify when doing --list, because browserifyinc just ignores this flag and always outputs the bundled source no matter what.

I don't know if this is technically possible or even something you want to support, but maybe adding some caching magic to the --list call and making browserifyinc run it (either outputting to stdout or respecting the -o flag, but it doesn't matter) would be great and might even shave a few more seconds off the entire process.

In https://github.com/browserify-rails/browserify-rails/pull/35#issuecomment-72001058 we were also discussing potential ways to cache the --list results to avoid running it unless strictly necessary, but that's way more difficult because you would have to accurately predict when a dependency has changed and avoid all weird edge-cases. Either way, I don't think any of the options we have rules out the other: if browserifyinc implemented --list we could still put a caching layer on top to squeeze a few more milliseconds out of it.

Let me know what you think, and thanks for the help so far!

jsdf commented 9 years ago

Okay I just published v1.4.0 which should make most browserify command line args like --list work

gabrielecirulli commented 9 years ago

Woah! I'll try it now.

gabrielecirulli commented 9 years ago

4 SECONDS!

You're my hero, this is wonderful! The whole thing went from 12 seconds to 4. I'm going to start using this in production to take advantage of the major speedups. I don't expect any major problems to arise, since from the limited testing I've done so far it seems great!

Thanks so much again! I really, really appreciated your responsiveness and help!