Fix issues with Underscore in the asset pipeline

[andy-armstrong]

https://openedx.atlassian.net/browse/FEDX-121

The previous approach for handling NPM assets was to symlink them into the static directory. This appeared to cause trouble with the asset pipeline where the files in question were not installed and then old versions were picked up instead.

This change instead copies NPM libraries to a new static directory so that the pipeline can consume them as with any other file. This new directory is added to .gitignore so that the files don't get accidentally checked in.

Note: I originally tried the change in pavelib/prereqs.py but it turns out that sandboxes don't install prereqs in the same way. It turned out to be simpler to instead copy the files during update_assets which is slightly inefficient (they don't need to be copied every time) but for a few files it doesn't seem like a big issue.

Sandbox

• https://andy-armstrong.sandbox.edx.org

Testing

• Manual verification of LMS and Studio on devstack and on my sandbox

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

• Code review: @cahrens
• Code review: @dan-f
• DevOps review: @feanil

FYI: @AlasdairSwan @sanfordstudent @maxrothman

Post-review

• Squash commits

[andy-armstrong]

Hmm, moving the asset copying broke both the Paver tests (as it should) and broke the JavaScript tests (I think because update_assets doesn't get called in that case). I'm on it...

[dan-f]

Was this supposed to be 'common/static/common/js/vendor'?

[andy-armstrong]

Good catch. Indeed it was!

[dan-f]

@andy-armstrong I think it's possible that we can avoid both symlinking and manually copying files around by creating a node_modules folder somewhere inside of common/static/js/, and then passing --prefix common/static/js when installing our front-end packages.

We can then hopefully use the default prefix (.) for all our server-side JS.

https://docs.npmjs.com/files/folders#more-information
https://stackoverflow.com/questions/14469515/how-to-npm-install-to-a-specified-directory

[andy-armstrong]

@dan-f Thanks for the suggestion and for the links. I don't know that we want to have everything from node_modules available through the static root (and hence on the CDN). It seemed cleaner to me to just install the handful of files into a central place once. I'd also be nervous about the extent of this change as there are a lot of libraries currently installed in node_modules. Would you be comfortable just going with what I have so that we unblock tomorrow's release?

@tobz Do you have any advice as to how best to handle npm-installed libraries? ... other than to switch to WebPack, of course :-)

[andy-armstrong]

FYI I've kicked off a clean sandbox build to verify that my latest changes are working. It will presumably be down until ~2:30pm.

[feanil]

I recall at some point the openedx was the new common? Is there a reason to put this in common vs openedx?

[andy-armstrong]

That's a great point. At the moment openedx is a purely Python package, so has no static assets. It would be good to move all this over, but maybe not now.

FYI, these vendor files get bundled in production so nobody will see the common paths to them. So it shouldn't be a big deal to move them in the future.

[dan-f]

Let's definitely unblock the release. I'd love to figure out a longer-term solution as well though so we can minimize additional steps in the pipeline as well as maintaining duplicate lists of libraries.

[feanil]

Relying on timestamps to do copies seems like a bad idea. There are lots of cases where the timestamps may not have updated correctly causing unexpected behavior.

[andy-armstrong]

This seemed like a good idea to save copying time, but I hadn't considered the possible operational impact. Should I switch it to force copying instead, or I could remove the directory as the first step.

[dan-f]

@andy-armstrong how does this approach handle libraries with dependencies? Part of the reason that we have a ton of files in the node_modules directory is that npm is a package manager, and each node module has a dependency tree. Does this approach require us to explicitly list - in paver - all the dependencies of a node package? What if that package updates (we allow minor or patch level updates using ~ syntax in package.json) and adds a dependency? I could see that breaking a build.

@feanil is there an operational problem with serving up everything in node_modules? Do you happen to know how folks deal with deploying apps that consume npm packages in general?

[andy-armstrong]

@dan-f Good question. There is no dependency analysis here, so every file that we actually use needs to be listed. I was thinking that we'd just be using it for simple pre-bundled libraries like JQuery and Underscore, but it is possible we'd use more heavily bundled assets in the future. My hope is that when we switch to Babel or React or something else that we will be using WebPack instead of these cobbled together home grown scripts.

[cahrens]

Perhaps rename STATIC_VENDOR_DIRECTORY to NPM_VENDOR_DIRECTORY since it is important that don't mix checked-in files and npm installed files?

[cahrens]

@andy-armstrong I reviewed the PR as it stands now, and I'm OK with this approach. Though if we need a temporary change to unblock the release, we could also just (temporarily) check the new version of underscore.min into the vendor directory.

[andy-armstrong]

@dan-f @feanil I've fixed the issue with running Jasmine tests, and written unit tests for the new behavior (and the old, since there were no tests for the Paver commands). I've also updated the sandbox and verified that the correct version of Underscore is bundled and that it is behaving correctly on both LMS and Studio. Please re-review when you have a chance. @cahrens has given a general thumbs up to the approach, and my only changes since her review were to fix the Jasmine tests and write new ones.

@feanil @maxrothman can we discuss the options for verifying the prod behavior. Since the release is blocked by this issue, maybe the best thing is to have @sanfordstudent cut the release and I can verify the fix on stage. If you think that's too risky then we'll have to hold the cutting of the release until we've done separate prod-style testing. Our final option is the one suggested by @cahrens, which is to replace the npm installed version of Underscore with a checked in copy of the correct version. This would just be a hack to unblock the release if we think that is less risky than what I'm proposing here.

@benpatterson FYI, you may want to review the Paver changes and tests too.

[cahrens]

👍

[benpatterson]

I realize paver is inconsistent on this, but the best practice is to use a python library to accomplish this. mkdir_p is called a few times in this file so I'd use that.

[benpatterson]

^ That doesn't need to hold up a merge, but I think it should be done in a near-immediate follow-on PR while we're here.

[benpatterson]

@andy-armstrong Looking over the paver testing side, I think you can avoid some of the dry_run logic if you use the @command_args decorator on the test_js task. I was just looking at it the other day, and that was what I had to do in order to prevent sh from actually running. Not sure if you got a look at that or tried it as part of this work.

[andy-armstrong]

@fredsmith this is the PR that you were discussing with @feanil. I'd love your feedback on his questions around prod configuration. Thanks!

[andy-armstrong]

@benpatterson Thanks for the feedback. I've addressed them and pushed up a commit.

[feanil]

Talked in person, Didn't realize that this was already tested on a sandbox. 👍

[andy-armstrong]

@benpatterson BTW, I don't think I can use @consume_args as it conflicts with @cmdopts which is already added to the tests. Note that the dry run logic I added wasn't around handling of sh (that was a cut-and-paste error as you pointed out). It was to indicate that other OS level operations were happening.

[dan-f]

👍 for unblocking the release.

I'd also like to discuss a longer term solution. I'm not sure how switching to webpack or ES2016 will answer the question of where node packages should be installed/how to serve them in production.

[benpatterson]

thx for the changes @andy-armstrong. We discussed the sh calls being suppressed during unit tests separately. Sounds like we're good to go there.

LGTM 👍