Nice, this seems great.

Yes it could be nice to get some numbers as performance is the primary concern here. It looks like the way things are going is that we could compile modules at install time with the System.registerDynamic wrapper so that this only runs for my custom code that I write in CommonJS and not other installed code.

I'd be happy to take this though soon.

I think this covers really well, in due course we can cater to ES6 template literals in a similar way. The hard part is working out if there are any tricky interaction cases between the three - comments within strings, strings within template literals etc. If we can handle each case based on what is sensible for CommonJS requires I think we can keep to the 99% goals pretty well without tokenizing.

Another option would just be to switch to a simple tokenizing system for all this. It would be interesting to see how that compares for performance if we could do all these functions in a single parse.

An earlier version of the loader actually did this stuff with tokenizing -

Extracting and caching dependencies offline seems ideal, as IIUC that'd
mean the dynamic detection would really only be applied in "dev" (not for
jspm installed code, not for code from the CDN, not for bundled code)?

That said, given that this PR fixes a live issue that prevents the use of
some packages, would it be acceptable to merge it as a stopgap? I'd
understand reticence if this is going to perceptibly affect performance
though - I could try to run some jsperf tests, eg running that code on a
really large CJS file with and without the fix to measure the impact?

Might have some time this eve.

Yes exactly, so we move towards having a dev loader and a minimal lightweight performant production loader.

My only reservation is whether we actually need to be doing tokenizing where strings meet comments. (think "//not a comment").

But we can always add this in at a later stage. Happy to take this for now.

Had a play with some completely non-scientific tests on jsperf:

http://jsperf.com/cjs-string
(running that code on the AngularJS source)

According to these, assuming I've not screwed up the tests (which would be somewhat astonishing), and assuming jsperf gives valid results, both in Chrome and FF the code after my change performs ~25-30% slower (on a beefy Macbook Pro).

Now to put the results in perspective, running this code on the angularjs source takes ~2ms before and ~3ms after the change. Seems generally fast enough, hopefully even on lower spec hardware, that the impact of the change isn't worth worrying about.

@theefer thanks, and the AngularJS source is pretty big, so this can still handle hundreds of modules pretty easily it seems.