More granular configuration

[cymen]

I need to add some tests for this which I'll work on later this evening but what do you think of this? You can try it out by using the more-granular-configuration branch of my fork of browserify-rails-sample:

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

If you access /beep, the page loads robot.js and beep.js (you can also access /boop). The two browserified-file contents are:

robot.js:

require=(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);throw new Error("Cannot find module '"+o+"'")}var f=n[o]={exports:{}};t[o][0].call(f.exports,function(e){var n=t[o][1][e];return s(n?n:e)},f,f.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})({"9WgEjc":[function(require,module,exports){
module.exports = function (s) { return s.toUpperCase() + '!' };

},{}],"./robot.js":[function(require,module,exports){
module.exports=require('9WgEjc');
},{}]},{},[])
// omitting rest of file (source map)

beep.js:

(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);throw new Error("Cannot find module '"+o+"'")}var f=n[o]={exports:{}};t[o][0].call(f.exports,function(e){var n=t[o][1][e];return s(n?n:e)},f,f.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})({1:[function(require,module,exports){
var $ = require('jquery-browserify'),
    robot = require('./robot.js'),
    says = 'beep';

console.log(robot(says));

$('document').ready(function() {
  $('#root').html(robot(says));
});

},{"./robot.js":"9WgEjc","jquery-browserify":2}],2:[function(require,module,exports){
// omitting rest of file (jquery-browserify and source map)

And the configuration is actually simpler than I thought:

robot:
  require:
    - ./robot.js
beep:
  external:
    - robot.js
boop:
  external:
    - robot.js

I need to try it with a slightly more complex example however it works great when moving from /beep to /boop (robot.js is same so cached, etc).

Fixes #16

[cymen]

I hit what I think is a bug with current node-browserify (3.39.0) -- actually, maybe all versions:

https://github.com/substack/node-browserify/issues/724

When we use browserify like we are by piping the Sprockets output, it behaves differently from when the input is a command line option. This is specifically when --require option is used. It appears to be a bug.

[cymen]

We also hit another bug with --list -- it works on node-browserify up to 3.36.1 but not any newer version: https://github.com/substack/node-browserify/issues/725

[cymen]

I've fixed the bugs in node-browserify and got them merged in so for node-browserify 3.41.0 it all works!

[hsume2]

@cymen thanks for working on this. Looking good so far. A few thoughts. Would it be easier if the configuration was closer to the way browserify works?

robot: --require ./robot.js
beep: --external robot.js
boop: --external robot.js

Also, what do think about adding some tests to cover this behavior?

[cymen]

@hsume2 I'm definitely going to add tests.

I don't think it would be good if it was closer to how browserify actually works because right now we support any command line option (well, the long variant of it with the -- prefix). In other words, although the syntax looks different, it maps over exactly (to my understanding).

[cymen]

@hsume2 The problem right now is that the global configuration conflicts with the bundle-specific configuration (this branch). It might make sense to fork this project and go with another gem name. This configuration file supports doing transform and fast and other options on at the bundle. The current master applies it to all calls. It depends on:

• Does your project have only one bundle?
• Does your project have multiple bundles?

For the first option, the global is fine. For the second option, it is not. This branch works for both. Master will sort of work for both but it might get confusing if people use a global configuration and a local configuration.

[cymen]

I'd actually hate to fork though. So maybe having both would be okay. Let's just see how it goes. I'll work on the tests.

[cymen]

@hsume2 I finally got a chance to take care of the tests and rebase my work to master. Can you take a look and see what you think?

This is now a critical part of my main project and I'll use test-first on future changes. It was hard to test-first when I didn't know how the various pieces worked together (rails, browserify, asset pipeline, etc). Now it is clearer.

On a side note, unrelated to this PR, if you're happy with this, can we move the main repo off to an organization? I created this:

https://github.com/orgs/browserify-rails/dashboard

I am not interested in taking control or anything like that. I do want to make sure we stick around no matter what and the different parties that have a big stake in using this gem can all come together and work on it.

[martenlienen]

I am currently reviewing this.

First I think instead of manipulating NODE_PATH to require modules by absolute paths, you should extract them into real node modules in <project root>/node_modules. Is there a reason, why you cannot do this?

Second as we are programming a rails integration, we should also use the rails logging instead of just puts.

Third you should use do..end blocks for multiline blocks.

Is there a special reason, why the hash key is not a symbol here?

I am still looking at the other changes and will comment more, when I have more questions.

[martenlienen]

Please prefer " over '.

[martenlienen]

If you put your code into it's own node module, you can also make use of browserify's support for configuration via package.json.

I thought, there also was some type of browserify.yml file to pass default options to browserify, but I cannot find it right now.

[cymen]

First I think instead of manipulating NODE_PATH to require modules by absolute paths, you should extract them into real node modules in /node_modules. Is there a reason, why you cannot do this?

The README.md has details on why. In short, jasmine-node expects to be able to move specs to a different level of nesting which breaks relative requires if you want to test something that is not extracted to a module. It also explains it isn't a good idea for production code, etc.

[cymen]

@CQQL Can you comment directly on line numbers? It makes things much easier.

[martenlienen]

In the end I would like to say, that I see the possible necessity of your changes and that you definitely need these features (and I might, too, in the near future), but I would rather solve these things with existing node.js and browserify-mechanisms and the example from the readme with app_main and app_secondary sounds exactly like they should be put into node_modules as own modules. So all "common" and reusable code should be moved into modules, while only the really view specific code should remain in app/assets. We are currently doing it this way with a react-based application and have all components in modules. It gave us a better structure to group code by functionality instead of by views and this in turn promoted code reuse.

[cymen]

Second as we are programming a rails integration, we should also use the rails logging instead of just puts.

That is great however I need a way to log based on the browserify.yml configuration in both test and running in actual rails. Rails.logger doesn't make this easy. I'll rip it out.

[martenlienen]

The problem is, that I cannot comment on the diff view. But I will try.

[cymen]

@CQQL You are thinking of starting something from scratch using browserify-rails. Think about it from an existing Rails application that you want to start applying browserify to. Your approach saying to do it a certain way makes it take much more work to transition. To even see if it is worth it.

There are more than one ways to use this gem. If we can't agree that this is valuable, then we'll have to fork. This is critical usage to the project I am on.

[cymen]

The problem is, that I cannot comment on the diff view. But I will try.

Ah! Okay, that is frustrating. Why are you reviewing this as if you owned the gem?

[martenlienen]

You should use rails' environment specific config files instead of loading different config files yourself.

[martenlienen]

Because I have push rights to the repo and rubygems and rewrote this gem into it's current version ~2-3 months ago :)

And I introduced this myself on our main project, because javascript code was getting out of control without some good module concept and I like node.js' a lot. That was also the reason, why I contributed to this in the first place. Before the functionality was a lot more basic.

[martenlienen]

As a workaround until you have refactored your code, you could also put a symbolic link to your assets into node_modules.

ln -s app/assets/javascript node_modules/my_app

And then you should be able to require("my_app/some/sub/modules"). But don't forget to put a package.json into your assets directory then. Otherwise browserify won't recognize it as a module, I think.

[cymen]

@CQQL What this PR does is add much more control over how browserify is used. We leave up to the developer to choose the balance between NodeJS and Ruby on Rails. I think it is right to leave that choice up to the developer and not try to force people to do it the NodeJS way when they are using Ruby on Rails.

Look at how old this PR is. I also obtain the rights to use browserify-rails gem name.

I am using this in practice for a RoR project that was around for quite a while. All of the code in this PR is to make it possible to slowly transition to the NodeJS approach. To make it possible in a number of rewrites going from one side to the other. That is exactly what I am using it for right now.

[cymen]

You should use rails' environment specific config files instead of loading different config files yourself.

Instead of using config/browserify.yml? That is wrong -- this is a gem configuration not an environment configuration. Using a YAML file for this is appropriate.

I can't really tell what you're responding to though. Taking a guess, maybe if the source maps should be enabled or not? That I agree would be appropriate to put in an environment file. I'll make that change.

[martenlienen]

At the moment I just do not feel, that the code is ready to be merged into master yet. It is somewhat hacked together just enough to make it right for your case. That is totally fine and I do have code like that too in my forks, but this gem is not just for you and me, but hopefully more people are using it and that seems to be the case. So when code is actually fed back into the main project, it should be consistent and above prototype quality.

If you really want to merge this, please do the following improvements:

• Use do .. end for multiline blocks
• Use " instead of '
• Use rails logging
• Use rails configuration. Using it is very appropriate as we are already using the rails configuration. You probably already configure several rails-integrated gems in your rails config.yml like i18n, actionmailer, activerecord etc. If we were not developing a rails plugin for browserify but something standalone, I would totally support our own config system. But then we would not use a Railtie for configuration.

I would like you to try out working with symbolic links.

Thanks for your engagement, cymen.

[martenlienen]

Yes there is config.yml for general configuration. But when you are loading a default configuration and a special test configuration, it looks like you are looking for environment specific configurations.

[cymen]

@CQQL @hsume2 I have removed you as owners of on the browserify-rails gem. I'll be forking and going my own way on this. Thanks.

[martenlienen]

That escalated quickly.

[cymen]

@CQQL I'm not very happy with how you handled this. Your behavior makes me worried to have you as a maintainer of any project I depend on. I am very happy to make changes to a PR to resolve any issues. This PR was begun to act as a form of discussion to make sure it got into master. In the meantime, you appear to have assumed ownership.

I highly recommend you reevaluate how you work with other people. A PR to satisfy one parities concerns can be extremely important to them. Your ideological assumptions about how people should modularize their code is not always relevant in day to day use of a project.

You appear very much to be assuming everyone using browserify-rails will be starting a fresh project with it. A seasoned project that was using other techniques is going to need measures that make it easier to move to another approach like going from the typical Sprockets approach to browserify. You do a disservice to dismiss code you do not find immediately useful in your own work. That is a real sign of developer immaturity.

The code in this PR is not of the highest quality in some aspects. You may have religious adherence to Ruby ideoms and be offend that I, for example, might use single quotes instead of double quotes. I care about practical solutions to problems. I am very happy to refactor my code. This emerged out of an obvious need. A need that I feel you have dismissed.

Finally, you need to be able to comment on line numbers on a PR. If you cannot do that, you should not be commenting. The reason for this is that typically a PR comment is resolved with a force push and github will mark the comment as "on an old piece of code" or similar. It is an excellent way to address issues that come up in a PR and by just bombarding me with comments, you make it very hard to resolve and address the issues one by one.

@hsume2 Are you happy with CQQL/Martin's handling of this PR? Please let me know. I am willing to work further on this and try to avoid forking. But I would like some understanding that we all have needs and that ideological-based reasons to not do something are going to get in the way of having a code base that people can use as they see fit.

[martenlienen]

First I would like to apologize for not commenting directly on lines. I have already commented on lines in PRs a lot, but somehow I thought that you could not, because this is basically a big compare view and you cannot comment on them.

How would you like me to adapt my interpersonal work/communication? I did

• Look into your needs and recommend approaches, that use already existing mechanics, so that we do not have to reimplement it, but you did not like those
• I listed requirements for your code to be merged. These requirements are that
  • your code adheres to the coding style of this project
  • builds on already existing and already used and fitting mechanics (like rails' configuration system) instead of building orthogonal ones

Is this unreasonable or unfair? At which point should I have done something else?

Furthermore I am not assuming, that this gem is not introduced into existing gems . I did so myself in a project, that has been developed for several years by multiple developers.

You agreed, that the code in this PR does not adhere to our coding standards in coding style as well as in previously mentioned aspects. That is totally okay in your own fork, where you can do whatever you desire. If it fits, do it. But how can you expect me to merge this? You also agreed, that your code is extremely important for you. I see, that it may be useful to other parties as well but they are also served well by a well maintained code base with consistent code quality and consistent use of rails' features. And maybe the other more browserify-/node-centric approaches solve their problems, which I would prefer.

Again I want to insist, that I have never dismissed your needs. You can require on git repositories in your gemfile and so there is absolutely no problem with adapting this gem's code to your special needs in your fork. But then, why does code, that is for your special needs, have to be merged back into the code base, that is for everyone's needs?

Let me paraphrase your arguments; be please do not be offended, I try not to be sarcastic.

• I have written this code and want it to be merged back into upstream
• I do not like your other solutions (because they are not solved with your code? I do not understand, why you do not try them)
• Code quality is not important and I want to merge my code as is

Lastly I would like to ask you, why you are summoning @hsume2 all the time? It is not like I ripped this gem out of his hands. I send several pull requests, that he liked, and so shared ownership with me. I have also merged other's pull requests and closed issues in the meantime. What do you want him to do? Should he say "CQQL, I remove you from your office as a maintainer, because you refuse to merge code, that does not fit in this code base in your eyes" and merge your PR? Who is going to further maintain this gem, when I am gone? You?

[martenlienen]

I just saw, that you created a browserify-rails orga and pushed to it's repository. Why can't you be happy with having your special solution in your own fork. Why do you have to force it on others?

[martenlienen]

Now I am the one, who is not happy with how you handled this. I feel like a victim of identity theft. You already have a fork on your account. But then you created the organization and you are impersonating this project.

You are absolutely right, if you are saying, that there is nothing, I can do about it. If you are unhappy with me, create a fork and push it to rubygems, but please rename it, to make clear, that this is a different project. Right now it looks like browserify-rails/browserify-rails would be the project home of this project, but it isn't.

[cymen]

@CQQL

1) A PR is a pull request. During a pull request, there are often little things that need changes. So cleanup is not unexpected. I am more than happy to adjust the code to use do and end on code blocks and so forth.

2) You constantly judge my use of this gem based on your use. This is not productive. Do not force me to work a certain way. Browserify supports multiple bundles:

https://github.com/substack/node-browserify#multiple-bundles

This PR makes it possible for people to use browserify how they would like to. It opens it up. You might find that ugly but that doesn't matter to me.

3) This project was started by @hsume2 and is still on his Github. You should defer to allowing him to provide feedback before "jumping down my throat."

4) The gem was originally under the name hsume-browserify-rails. I was able to get us the ability to use browerify-rails. In the meantime, you've jumped on board and asserted yourself in a way that I do not think is acceptable. If @hsume2 is not interested in maintaining this project, I am happy to assume maintainership over at https://github.com/browserify-rails/browserify-rails. You are of course free to use another gem name.

5) You did a particularly bad job in providing feedback mixing up a number of concerns and putting them all in direct comments which are difficult to address. That is why we comment on specific line numbers so the discussion can happen in line and the discussion gets expired when the code is updated to resolve whatever the discussion is about. This works well. It seems to be the Github standard.

6) Thank you for the feedback that was factual and code-based. I've made some clean up in those areas before merging my PR in on the fork of this project.

7) I am waiting to hear back from @hsume2 on how he wants to handle this.

So in summary, I am responding in kind to your behavior to a certain degree. As I secured the ability for us to use the browserify-rails gem name, I am comfortable continuing to hold on to it. I am more than happy to have pull requests but you will not have any special rights on that repo.

[martenlienen]

Just as a reminder, I never said, that I would not merge this. I told you four improvements, of which you implemented two. But you are unwilling to implement the other two. If you did that, this PR would have been acceptable.

I will continue to develop this gem and you can proceed however you like with your forks.

[cymen]

@CQQL What are the other two improvements? I am aware of these two -- I'll reply here (this is why comments on the code are much easier to deal with):

1) Using symbols instead of strings when doing key looks in a YAML file does not work correctly. They do need to be strings and not symbols. Maybe there is another way we can load the YAML file that converts the strings into symbols however the default result of the YAML load does not convert the string keys to symbols.

2) Using Rails logger is problematic. In the dummy environment, it isn't possible to get logging output even if you set config.log_level = :debug in the dummy config/application.rb (or appropriate environment file). Do you know how to get the logging working where we can log both in Rails at the debug level and to the console when running tests? If you know a way, that would be awesome but so far I haven't found a working approach. We could have two logging calls -- when to Rails.logger.debug and another that somehow detects it is running in the dummy and logs to console (this is easy to do). I was hoping for something cleaner.

If the improvements were other ones, please let me know -- I'm more than happy to implement them.

It was not clear at all you were willing to merge this. Let us see if we can resolve this conflict by my making this PR acceptable to you.

[martenlienen]

Which dummy are you talking about? Do you mean the test application in the test directory? We should use the rails logging either way. If there is a need to see the output during test runs, we may use puts or similar, but that should not be committed. We should not litter application logs with our debug statements. I will look into that this weekend.

The other improvement is, using the rails configuration mechanism, that we are already using to configure this gem, to configure this gem ;)

I will do some commits myself this weekend, to show you, how I would do it.

I am sorry, that I cannot work on this right now, because I am invited somewhere and will be quite busy over the rest of the weekend, but I will try to squeeze it in somewhere.

[cymen]

I am talking about the dummy application in the test directory. This feature is mainly to help with development but it is also useful for consumers of browserify-rails. We need a way to log what actual browserify command is being run. It needs to be possible to log that both in tests and in production. This gives us that feature. We can figure out the appropriate way to get it to work but it doesn't work out of the box in the dummy application.

Using a YAML file for configuration is normal. If you work on a large application, having massive application.rb files is not very useful. Putting these commands into environments might work but that is going to duplicate configuration. For somewhat complicated interactions like browserify-rails, having a separate configuration file is not against community patterns in Rails. It is ordinary. So I'm willing to hear what you suggestion is but unless it is clearly better, I think it is cleaner to have config/browserify.yaml.

[cymen]

I forgot to say I do think it is a little confusing have configuration in two places -- I'd lean towards extracting all of the configuration that is global to browserify.yml. So the command line directives I'd move to browserify.yml. The only thing I'd leave in the Rails configuration files is environment-specific things like enabling source maps.