Upgrade Underscore.js and Underscore.string.js

[cahrens]

FYI @andy-armstrong and @dan-f, here is our feature branch.

[andy-armstrong]

FYI @symbolist

[andy-armstrong]

We hit this known BokChoy flaky test: https://openedx.atlassian.net/browse/SOL-430

[andy-armstrong]

jenkins run bokchoy

[cahrens]

@andy-armstrong Here is the PR where the underscore.string.min.js upgrade passed all tests-- https://github.com/edx/edx-platform/pull/11632

I looked at the code and don't see a difference. I'll try rerunning the JS files to see if it is a timing issue. For reference, here is the link to the failure: https://build.testeng.edx.org/job/edx-platform-js-pr/14035/

[cahrens]

jenkins run js

[andy-armstrong]

@cahrens I rebased the branch and it is now green again, so I don't know if this means that we have some flaky tests or whether something legitimately changed.

BTW, I'm thinking of merging this branch to master next week and getting it out in the following week's release, rather than waiting for the JQuery upgrade. I think this will minimize the risk, and I've discovered that it is necessary when using the UI Toolkit in edx-platform just to be able to use some of the utility methods. In order to do this, I have to finalize the testing plan and communicate it with edx-code and with the course teams. Let me know if you see a problem with this approach.

FYI @AlasdairSwan

[andy-armstrong]

@cahrens I've got a little further investigating the test failure, since it is happening all the time on my branch. There seems to be something wrong with this line in edX Notes where it calls _.wrap:

https://github.com/edx/edx-platform/blob/master/lms/static/js/edxnotes/views/shim.js#L254

The Underscore function only take two arguments, but for some reason the Notes invocation passes three. When it fails, _.wrap ends up returning a string rather than a function, and then obviously havoc breaks out when that is installed as an attribute that is expected to be a function.

I only see two calls to _.wrap in edx-platform, and the other one appears to be fine. I'm going to keep investigating...

[andy-armstrong]

@cahrens I've found the Underscore change where _.wrap no longer obeys multiple arguments (GitHub blame is my friend). It was from October 2013!

jashkenas/underscore@74a747c

Now to work out how to rewrite the Notes function to preserve the intent.

[andy-armstrong]

@cahrens I now believe that there's a problem with Underscore.string which is overwriting _.wrap with its own version that returns a string. It seems that this might be due to your change to Underscore.string in this PR:

esamattis/underscore.string#489

If this is the case, I'm not sure why the other caller isn't having trouble too:

https://github.com/edx/edx-platform/blob/master/cms/static/js/views/baseview.js#L30

I suppose it must be the case that Underscore.string isn't being brought in by Studio, at least in the cases where the base view is used.

I'll investigate more in the morning as I'm out of time for tonight.

[cahrens]

@andy-armstrong I'm sorry you've been wasting time on this! Yes, wrap was overwritten by underscore.string, and the PR I submitted fixed the issue by ensuring that the underscore.string wrap function is excluded by exports.

This was released as version 3.3.4, and it fixed my issue. It must be that somehow code is adding underscore.string to the _ namespace without using exports. If you are adding underscore.string methods to _ (which actually isn't recommended anymore), this is what the syntax should be:

_.mixin( _s.exports())

[cahrens]

This should be 3.3.4! Somehow the wrong version cropped in?

https://github.com/epeli/underscore.string/blob/master/CHANGELOG.markdown

And look, it was correct here:
https://github.com/edx/edx-platform/pull/11632/files

I am at a loss as to why this branch keeps losing changes.

[andy-armstrong]

Thanks @cahrens. I've fixed the version and squashed it into the original commit. If this fixes the build both here and in my PR then I plan on merging this PR so that we're ready for the XSS marathon on Wednesday. I'll do so tomorrow after the release is cut to give us a week to settle in to the changes. Let me know if you see an issue with doing this.

[andy-armstrong]

@cahrens Any objection to me merging this?