CommonJs loader does not ignore requires() in comments, if they appear at the last index of comments

[colmbrady]

It seems that the CommonJS loader can erroneously detect a module dependency which is actually not a correct dependency and is infact a comment block.

Im not quite sure how I can submit a unit tests to prove this as i'm using SystemJS to load up a browserified Javascript bundle, which contains a combined list of dependencies, one of which is handlebars. (The bug manifests with handlebars)

Anyhow, the module 'amddefine' (a dependency of handlebars) has the following comment -> '//Synchronous, single module require('')'. It seems SystemJS is recursively walking the dependency tree for all modules looking for "requires" -> because this comment has a require('') at the end of the comment line a string match bug causes SystemJs to think that it must require(''). This is the bug.

The bug currently exists at this line in master: https://github.com/systemjs/systemjs/blob/master/lib/cjs.js#L27

The code should be as below, (notice the match[0].length -1)

function inLocation(locations, match, starts) {
      var inLocation = false;
      for (var i = 0; i < locations.length; i++)
        if (locations[i][0] < match.index && locations[i][1] > match.index + (!starts ? (match[0].length - 1) : 0))
          return true;
      return false;
    }

Im happy to submit this fix in a pull request if you can provide some guidance R.E how to unit test this in your codebase?

Ive attached the javascript file but as its browserified i'm not sure how useful it is.

[colmbrady]

bundle.txt

Searching for require('') in this file shows where the issue is occurring.

[guybedford]

Amazing thank you so much for the detailed report!

The test cases for this are in the file at https://github.com/systemjs/systemjs/blob/master/test/tests/commonjs-requires.js. If you're able to provide a PR that would certainly be welcome, feel free to add an additional line there.

[colmbrady]

master...colmbrady:comment-detection-issue

Ive got some test coverage on the issue.

[guybedford]

Released in 0.19.7.