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

Sprockets-required files don't share their required modules #18

Closed
mattwondra opened this issue Sep 18, 2014 · 27 comments

Comments

@mattwondra
Copy link

commented Sep 18, 2014

It appears that when you use //= require to include browserify-able files, these files don't share their modules. For example:

//= require modules/a
//= require modules/b
// Both modules have: var x = require("../lib/counter");

In the compiled file, the counter library will be written twice. I know there are documented ways around this but it seems a huge burden to rely on this YAML file, especially in a large project with many files and multiple shared dependencies.

The problem I've seen reported is mostly to do with file bloat — you don't want large libraries to be defined multiple times in one file. But in some cases, it's actually catastrophic to initialize the same library several times on the same page. For example, React will refuse to work unless it's give a solitary definition/initialization.

I built a reduced test case and documented the issue here: https://github.com/mattwondra/browserify-rails-issue

Ideally, the postprocessor in this gem should be able to share the module definitions across //= require-d files. But if this is overly difficult or impossible given the Asset Pipeline's tools, may I suggest an optional strict_mode config flag that prevents you from //= require-ing Browserify-able files?

I built a local prototype and would be happy to submit a pull request. When this mode is set, the asset precompile fails if it detects that a //= require-d file needs to be Browserified. The error message tells you to require(...) them instead.

Thanks for the consideration!

@rymohr

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2014

How does the strict mode work? Are you just checking for the existence of a require() call?

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 19, 2014

Thanks again for quick replies on these issues!

As I said, the ideal fix is for scripts in different branches of the //= require tree to have knowledge of what modules have already been require()-d. But I'm not nearly skilled enough in Asset Pipeline to know if that's even possible.

For the "strict mode" flag, my admittedly naive implementation is something like this, in BrowserifyRails::BrowserifyProcessor:

def evaluate(context, locals, &block)
  if config.strict_mode.present? && config.strict_mode == true
    check_for_sprockets_required_modules(context._required_paths)
  end

  # ... do existing browserify checks
end

private

def check_for_sprockets_required_modules(required_paths)
  # Loop through all //= require'd files to see if any of them are Browserify-able modules. 
  # If they are, issue an error stating that these should be rewuire()'d instead.
  required_cjs_modules = required_paths.select do |path|
    file = File.open(path)
    contents = file.read
    file.close()
    # If the //= require'd file is a module or requires other modules, add it to the list!
    contents.include?("module.exports") || contents.match(/=\*require\(/)
  end

  if required_cjs_modules.length > 0
    raise BrowserifyRails::BrowserifyError.new("You cannot `//= require` any files which will be run through Browserify. Instead you should `require()` the following modules in #{file}:\n\n#{modules.each{|path| "#{path}\n"}}")
  end
end

In short, it looks through all files listed in context._required_paths, reads them, and looks for module.exports or = require(. If it finds those, it errors and stops the pipeline processing, noting that these files need to be require()-d instead.

A better implementation would be to use browserify --list like the dependencies method — but on my local system that command is taking 2–10s per file to run, so it would make the whole compile process excruciatingly slow to run on every single dependent file.

(On the fly idea: Maybe we could concatenate all of the dependencies and just run browserify --list on the whole shebang, so it only has to run once?)

The biggest caveat with strict_mode (or whatever you want to call it) is that you will only be able to require() modules from the top-level manifest file. Because if you require() anything down the //= require tree, strict mode will throw errors. So in essence, it breaks this idea of //= require and require() living in perfect harmony.

For my specific use case, that's not a problem — we're considering moving our app to a fully node-based asset management system, and see browserify-rails as a stepping stone to get there piecemeal. But for others, this solution is insufficient, and there really needs to be a way for files in different branches of the tree to share module information.

Hope this all makes sense, please let me know if I can clarify at all!

@mattwondra mattwondra closed this Sep 19, 2014

@mattwondra mattwondra reopened this Sep 19, 2014

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 19, 2014

(sorry, hit the wrong button and accidentally closed the issue — reopened due to #fatfinger)

@cymen

This comment has been minimized.

Copy link
Member

commented Sep 19, 2014

So the right way to do this is to use browserify bundles. That means you have to explicitly tell browserify what files/modules to mark as being reusable. Doing it automatically has some downsides. So to fix your case, I would create a browserify.yml file as discussed in the README. There is an example of that here:

https://github.com/cymen/browserify-rails-sample/tree/more-granular-configuration

Here is the config/browserify.yml file:

https://github.com/cymen/browserify-rails-sample/blob/more-granular-configuration/config/browserify.yml

And the files being referenced are in app/assets/javascripts as the root for the relative require.

@cymen

This comment has been minimized.

Copy link
Member

commented Sep 19, 2014

Whoops, I reread your first post. I agree, it is a painful method when you have very large projects with a large number of bundles. It is possible to automate what we are doing in the browserify.yml file. I actually used it so we could get away from multiple bundles as my use case was refactoring a non-modular code base to modular and eventually we didn't need bundles (where I am today on the main project I'm using this on).

An automated way to do it for your use case makes more sense. I'm not sure what that looks like though -- I've thought about it a little bit but I'm curious what others think.

However, if most people are using browserify.yml solution as a stop gap, I'd suggest this isn't a problem as it is a temporary annoyance. I think waiting for the person who comes along and actually needs this to go forward with making it a reality is the most realistic attitude. But it is certainly an interesting problem to ponder in the meantime.

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 19, 2014

@cymen Thanks for the thoughtful response. I'm fairly new to Browserify, so I definitely don't want to advocate for moving this gem in a non-Browserify-ish direction. Just trying to see if there's some way to ease the pain of my particular use case.

In the general use case, I'd still worry about creating havoc with more fragile modules, like React (which fails completely on being initialized more than once). Using bundles to manage the dependency won't protect against developer error — forgetting to mark React as external when you create a new dependent file. But — if this is a problem with vanilla Browserify anyways, I'm sure it's not the responsibility of this gem to fix it.

Thanks again, and I'd welcome any ideas or feedback. I'm not convinced that strict_mode is a good inclusion into this gem's core, and my team can always implement it as our own preprocessor no matter what. But thought I'd throw it out there.

@cymen

This comment has been minimized.

Copy link
Member

commented Sep 19, 2014

After thinking about this, I think the way to go might be to support opting out of automatic require/export but by default opt-in. So one would use browserify.yml for the exceptions. I don't know how to do this in a well performing way though but I'll think about it too.

In terms of the strict_mode, I'd definitely welcome any PR and I assume the rest of the team feels the same way.

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 29, 2014

Thanks again for all the feedback on this issue. But after careful consideration, my team and I have decided to go another direction. Instead of fighting to pull Browserify and other modern front-end tools into the Rails ecosystem, we're going to move our asset management completely off the Asset Pipeline and into a node-based system using grunt/gulp.

Since we won't be using this gem in production any longer, I don't think it's wise for me to champion strict_mode and issue a pull request. The concept is outlined in the above code, so anyone who finds this through feeling the same pains can take that and run with it.

@cymen

This comment has been minimized.

Copy link
Member

commented Sep 29, 2014

@mattwondra Thanks for relaying your decision. For the main project I'm using this on, we're planning on doing the same thing (getting out of the Rails asset pipeline). At least for the major "web application" part that I work on. The team is probably going to use this gem still for the JavaScript that is more of an augmentation (not so much application).

@rymohr

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2014

I feel myself being pulled this direction as well but haven't looked into what it would take to convert. Are there any good systems already in use or are you guys rolling your own?

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 30, 2014

We're rolling our own. In our research, we found plenty of people moving from Rails Asset Pipeline to grunt/gulp/what-have-you. But the main problem is that they all describe techniques to go whole-hog, which may work well if you're just starting a project, but was prohibitively expensive for a small team (2 front-end devs) on a large codebase without many automated tests (yet).

So we devised an interim solution, what we're calling a Hybrid Asset Pipeline. We're hoping to do a blog write-up on it soon (I'll link here when we do). But the basic idea is to introduce node-based asset management into the ecosystem, so that we can slowly move each piece of our app into it and then completely cut off the Rails Asset Pipeline. We're starting with transitioning JS only, but long-term we'll serve CSS and images through node as well.

What this looks like in practice is, we now have two JS manifest files. application.js is the standard Rails manifest, and //= require's all our existing code. And we have a new node manifest, bundle.js, which will require() all our new code using Browserify. We have a node watcher running in the background which compiles bundle.js into _bundle.js whenever there's a change, so that's how all our modules will be assembled. And then the compiled node code is included into the Rails Asset Pipeline with //= require _bundle in application.js, so that both pipelines feed into one final compiled file, served through the Rails Asset Pipeline.

Here's a flowchart we made to illustrate this process:
hybrid asset pipeline flowchart

There's a little more to it than that — for instance, to get node pipeline errors to the browser, we have node write that error in a formatted /* */ block to _bundle.js and a custom Rails precompiler picks up on it and throws a precompile error. That same precompiler checks to make sure we're not using require() or module.exports in any //= require'd files.

It's not perfect, and it required us to add another process into both our development environment and our deploy scripts. But it lets us move piecemeal to a new system while still taking advantage of some of the niceties of the Rails Asset Pipeline (like <%= javascript_include_tag "application" %> and md5 fingerprints). Eventually we'll need to figure out how to simulate that in our fully node-based system, but at least we can focus on transitioning the codebase first.

Hope this is helpful! I still think the browserify-rails gem has a place for those working on smaller projects, but for us this is already working a lot better.

@litch

This comment has been minimized.

Copy link

commented Sep 30, 2014

@mattwondra That's great to read - I'm React-ifying a rails app myself and now at the point where it's time to start figuring out the CommonJS & Jest parts. I've been trying to understand how these "node-y" asset management will work with the Rails asset pipeline. The more I learn about the other solutions that are actively being developed by such big communities, the more it seems like the Rails asset pipeline should probably be transitioned from in the near future.

I've got more research to do on this front but really appreciate your insights here.

@rymohr

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2014

Thanks for the detailed write-up @mattwondra. Are the bundled assets all commonjs modules? Does your bundle builder use a cache or rebuild the entire bundle each time?

I'm still trying to figure out the best way to handle dependency updates without having to invalidate every file that depends on them: #14

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 30, 2014

They're not all commonjs modules. For example, in bundle.js we have:

require("./initializers/readers");

And readers.js just looks on the page for reader-related placeholder DOM elements, and inserts the proper React components in. It require()'s those React components, but does not export anything. (This may be an anti-pattern, but it's the way all our current code drops JS components into the page and we're trying to focus on one refactor at a time 😄)

Right now our Node Asset Pipeline is built using simple node scripts run with npm. The build script for deploys uses browserify to package bundle.js up, and our development watch script uses watchify. So whenever we change files anywhere in the bundle.js require() tree (like a component require()'d by initializers/readers.js), watchify re-builds the bundle. And watchify does handle its own caching, so only the changed modules are re-built.

I'm not sure how #14 factors in for us — we've completely dropped browserify-rails from our asset compilation so any external packages are build through our Node Asset Pipeline and then imported post-compile into our Rails pipeline. Is there something I'm missing?

@rymohr

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2014

The basic test would be to run npm update and see if every file automatically pulls in the updated package.

It sounds like watchify must be handling the cache invalidation for you. With browserify-rails you need to touch the dependent assets in development to pick up package updates.

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 30, 2014

Right, all of our require()'s are being served through the Node portion of our Hybrid Asset Pipeline, so the node-based tools know how to handle dependency changes. By the time Rails is processing assets, all of our JS modules have already been bundled up.

It sounds like the core issue you're having is related to the original issue in this thread — Sprockets processes assets individually, so they can't really communicate cross-dependencies with one another. In order for Browserify to really work properly, the dependent assets need to be able to be processed as one unit, instead of individually.

Off the top of my head, the best thing I can think of is to augment your npm update command so that after the update it also runs rake assets:clean. This should force the entire Rails Asset Pipeline to recompile every time you upgrade dependencies. It's not great, but it works — and without some serious hacking and probably fragile code, I'm not sure how you can automatically touch only the dependent files so that only those recompile. In theory you won't be upgrading npm packages enough for this to be a severe drawback. The biggest problem here is it adds an install step for gem users: "Add this script to your package.json".

In theory, the best way to do this would be to have a custom preprocessor that's run every time Sprockets checks if it has assets that need changed (I'm not sure if this callback exists). It checks node_modules for changes — if it finds them, it does rake assets:clean and thus forces the entire tree to recompile. The advantage here is that this will roll out to all gem users without adding an install step..

@rymohr

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2014

Browserify-rails and the asset pipeline do a great job with dependencies within the app itself. It's only these outside node_modules dependencies that throw things off.

When you have thousands of coffeescript files to process, having to run rake assets:clean can really slow down development. Especially if you're developing a standalone package in tandem with the app using npm link.

How are you handling templates?

@mattwondra

This comment has been minimized.

Copy link
Author

commented Sep 30, 2014

thousands of coffescript files

Yeah... That'll make a full recompile pretty expensive... :)

I'm assuming you mean JS templates? Our existing code is primarily Bootstrap using Handlebars for templating. In all our new code, we're moving to React for various reasons, with our templates written inline in the component files with JSX. We'll be transitioning everything over to React piece by piece as well.

It might be worth mentioning, my use case isn't a single-page app. I work for AT Media on the lifestyle blogs Apartment Therapy and The Kitchn. This is why a Hybrid approach is working well for us — we can build and move individual features into React on node without really affecting existing components. Other use-cases like single-page apps might have a harder time adopting this in-between approach we're taking.

@justin808

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2014

Here's an alternative solution that might be of interest:

Fast Rich Client Rails Development With Webpack and the ES6 Transpiler

@litch

This comment has been minimized.

Copy link

commented Oct 16, 2014

Yeah, I did the thing @justin808 had written up - and it worked like a charm. Can run jest tests, live-recompiles, etc. Much easier than browserify.

@rtorr

This comment has been minimized.

Copy link

commented Oct 16, 2014

@justin808 that is a nice write up. Will digest it and test it out.

@cymen

This comment has been minimized.

Copy link
Member

commented Oct 16, 2014

@justin808 I appreciate you posting that too -- I'm going to bring this up with our internal team for part of the project we're on that is going to stay in Rails (the part I'm on is temporarily using browserify-rails until we separate completely and no longer are in Rails -- we just hit a Rails-backed API).

@schovi

This comment has been minimized.

Copy link

commented Jan 30, 2015

Any progress with that? I am new with this gem and npm generally and this problem blowing my mind. I am using this for require('react-flux') and when it is loaded twice or three times it is broken, because there are 3 different instances of this library and they do not communicate (my explanation).

browserify.yml should somehow solve that, but i really dont know how. I require react-flux in these files assets/javascripts/app.js, assets/javascripts/actions/user.js, assets/javascripts/constants/user.js and assets/javascripts/stores/user.js

Or is there any better solution? Thanks

@cymen

This comment has been minimized.

Copy link
Member

commented Jan 31, 2015

@schovi There is a lot going on -- I started to type a reply and decided instead to create an example repo for using multiple bundles:

https://github.com/browserify-rails/multiple-bundles-example

Can you take a look at that and let me know if that makes it all mesh together?

@cymen

This comment has been minimized.

Copy link
Member

commented Jan 31, 2015

@schovi I should mention, react-flux is your ./robot in the example. That way you can have react-flux in one bundle and the other bundles can be marked as having it external so you'll only have one copy of it packaged up (but you'll have to ensure you load that bundle that does have react-flux marked as require before any other bundle that has react-flux marked as external with a javascript tag so in the HTML page).

Also note the issue with being stuck at browserify v4.x at the bottom of the README (at example repo)!

@schovi

This comment has been minimized.

Copy link

commented Feb 1, 2015

@cymen It seems to work, but

  1. In my app i have lot of dependencies. Only for react-flux it is 3 times per model (it has some structure) and i will have approx 10 models. So it is 30 files. Plus other libraries. It is not maintanable with this solution.

  2. This solution is not production ready. Separate files wont compile into one :/

I guess i will try to beat that Webpack solution linked there.

@cymen

This comment has been minimized.

Copy link
Member

commented Feb 2, 2015

@schovi

  1. We are using browserify and browserify-rails does no more than browserify itself would do. If your issue is with having to deal with browserify.yml due to wanting to have tons of bundles and having to mark shared modules as external, then yes -- it'll be annoying to maintain. But no more so than using browserify itself directly.

  2. We are using the Rails asset pipeline. All you have to do is bundle exec rails assets:precompile and away you go -- that is the Rails way to get your one file.

The sweet spot for this gem is for projects:

  • using the Rails asset pipeline (ie have engineers that don't want to go compile JS separately)
  • want one big JavaScript bundle
  • or those with multiple big JS bundles that are refactoring to one

Once you get into wanting to share a lot of modules, it gets more complicated. It isn't an exceedingly hard problem to fix but it needs someone to put in the time and in some cases some of the above sweet spots don't apply and other approaches make more sense.

If you are indeed compiling to one file you don't need to do require/external at all -- this project will work just fine for you.

@cymen cymen closed this Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.