Key words can't be used in comments

[matthewlehner]

Right now there are a few edge cases that will cause browserify-rails to fail in unexpected ways (which, because they're unexpected makes it hard to track down 😕)

JavaScript files including the words "module.export" or "require('something')" outside of the expected use – i.e. in a comment or string will stop browserify from working randomly, with no error leading to the problem.

Examples of cases that will cause failure:

// A comment which contains require('a-module')
// a comment which contains module.exports

/*
   multiline comments with 
   require('a module') or
   module.exports 
*/

// Strings
"that say require('something')";
// or
"you may want to use module.exports";

I've helped this slightly with #125, but I think there's still a ways to go before this is resolved. Unfortunately, because this is parsing JavaScript with Ruby, there's no easy way to tell whether or not these key words are within comments or strings.

We can do a better job with the regexes that check for their presence and should add this gotcha to a visible place within the documentation.

[cymen]

While I'd like it to be seamless, I'm also worried about the potential added complexity/runtime cost for proper detection. I go back and forth on whether it is worth it but I do suspect there could be a reasonable compromise (in terms of discovering if our triggers are in comments -- although if we do that, we need to ensure we have a magic string that we will match to accommodate those working around detection). So I'm definitely not ruling anything out and would hope to see better detection than what we have today.

[matthewlehner]

I've been giving this a bit of thought, and agree, seamless is better. I think the best way forward would be some additional regexes that are run after to ensure that the trigger is not in a comment, or a string.

Beyond that, we should try to raise an exception with a good error message when this occurs? I don't know enough about sprockets internals and the interactions between it and browserify works to know if this is an option, but it seems like it'd be quite helpful.

[cymen]

My way of thinking of Sprockets is we basically get stdin and stdout. The input comes from Sprockets after it has done some of it's things like //= require and such. We could do our additional checks and log an error message on false positives and just return stdin as stdout (basically, we wouldn't do any transform and just give Sprockets back what it gave us when we have a false positive). I'm not sure where the logged error messages would go (probably wherever Rails is configured to log stderr) and those errors would probably get overlooked but it would follow convention.

[matthewlehner]

Do we get exit codes? Or just the output?
If the processor raises an exception, the error message will show up when running rake assets:precompile, in the output from rails server, and in development, in the browser (I think).

[cymen]

We can cause an error -- there is an example of an exception thrown in the processor when it doesn't find the browserify commands. I was assuming we'd be open in what we accept and gloss over false positives (just log them) but maybe that isn't a good idea.

[matthewlehner]

I like this idea, if I get a few spare moments this week I'll send a PR.