Bug when concatenating sources

[cymen]

In some cases, it is possible to end up with a browserify-build file to have JavaScript that should have had a semicolon inserted between the two sources but did not. A quick hack to fix this (and prove this is the case I'm hitting) is to alter the last line of run_browserify to return stdout + ';' as shown here:

    def run_browserify(options)
      command = "#{browserify_cmd} #{options}"
      directory = File.dirname(file)
      stdout, stderr, status = Open3.capture3(command, stdin_data: data, chdir: directory)

      if !status.success?
        raise BrowserifyRails::BrowserifyError.new("Error while running `#{command}`:\n\n#{stderr}")
      end

      stdout + ';'
    end

On the one hand, spurious semicolons are not a problem. On the other hand, it is kind of ugly.

[cymen]

The question I have is do we want to be lenient in what we accept and fix this (perhaps being smarter when we put in the semicolon on) or do we want this to break and the source file needs to be fixed. I'll try reverting this locally and see what is causing the problem.

[hsume2]

@cymen thanks for bringing this up. I think it may get tricky figuring out if a final semicolon is needed. Perhaps a full approach would be some sort of JSHint integration. That way source files can be validated prior to concatenation (and the final source can be validated as well). What do you think?

[cymen]

@hsume2 I determined this is due to node-browserify not ending output with a ';'. I have a fix for this there:

https://github.com/substack/node-browserify/pull/730

[hsume2]

@cymen great! Once https://github.com/substack/node-browserify/pull/730 is merged we can bump the browserify version in the package.json.

[cymen]

@hsume2 It might end up not being accepted into node-browserify. We do know that it'll return code without an ending semicolon if it is not accepted. In that case, we should add the semicolon. I would understand substack going either way on it so let's see what is decided on that issue.

[martenlienen]

I will close this, because I cannot reproduce this and there does not seem to be an immediate need for a fix anymore.

[cymen]

@CQQL This was fixed with the PR I did to node-browserify to resolve the issue.