Fix issues with Underscore in the asset pipeline #11938

andy-armstrong · 2016-03-24T15:12:06Z

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 · 2016-03-24T17:25:46Z

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 · 2016-03-24T17:47:24Z

.gitignore

@@ -73,6 +73,7 @@ bin/
 lms/static/css/
 lms/static/certificates/css/
 cms/static/css/
+common/static/js/vendor/


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

Good catch. Indeed it was!

dan-f · 2016-03-24T17:54:13Z

@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 · 2016-03-24T18:01:51Z

@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 · 2016-03-24T18:06:45Z

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 · 2016-03-24T18:12:07Z

pavelib/assets.py

+]
+
+# Directory to install static vendor files
+STATIC_VENDOR_DIRECTORY = 'common/static/common/js/vendor'


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

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 · 2016-03-24T18:16:09Z

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 · 2016-03-24T18:16:19Z

pavelib/assets.py

+
+    # Copy each file (if changed) to the vendor directory
+    for library in NPM_INSTALLED_LIBRARIES:
+        sh('cp -u node_modules/{library} {vendor_dir}'.format(


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.

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 · 2016-03-24T18:22:39Z

@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 · 2016-03-24T18:51:21Z

@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 · 2016-03-24T20:31:41Z

pavelib/assets.py

+    'underscore/underscore.js'
+]
+
+# Directory to install static vendor files


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 · 2016-03-24T20:38:41Z

@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 · 2016-03-25T00:17:22Z

@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 · 2016-03-25T00:34:10Z

👍

benpatterson · 2016-03-25T12:13:17Z

pavelib/assets.py

+        return
+
+    # Ensure that the vendor directory exists
+    sh('mkdir -p {vendor_dir}'.format(


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.

^ 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 · 2016-03-25T12:19:15Z

@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 · 2016-03-25T13:35:53Z

@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 · 2016-03-25T13:37:16Z

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

feanil · 2016-03-25T13:43:45Z

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

andy-armstrong · 2016-03-25T13:55:07Z

@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 · 2016-03-25T13:56:20Z

👍 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 · 2016-03-25T13:59:57Z

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 👍

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.

benpatterson · 2016-03-25T14:06:40Z

also thinking longer-term, i'm not sure if browserify is in our package.json, but that is worth a look for local troubleshooting.

andy-armstrong force-pushed the andya/fix-underscore-on-sandboxes branch from e0586d4 to 5747da4 Compare March 24, 2016 16:31

dan-f reviewed Mar 24, 2016
View reviewed changes

feanil reviewed Mar 24, 2016
View reviewed changes

cahrens reviewed Mar 24, 2016
View reviewed changes

andy-armstrong force-pushed the andya/fix-underscore-on-sandboxes branch 2 times, most recently from 5de9313 to e865d8a Compare March 24, 2016 22:59

andy-armstrong mentioned this pull request Mar 25, 2016

WL-325: Multi-Site Comprehensive Theming #11885

Merged

benpatterson reviewed Mar 25, 2016
View reviewed changes

andy-armstrong force-pushed the andya/fix-underscore-on-sandboxes branch from bd99a32 to 6dd09a8 Compare March 25, 2016 14:02

andy-armstrong merged commit e8f620a into master Mar 25, 2016

andy-armstrong deleted the andya/fix-underscore-on-sandboxes branch March 25, 2016 15:16

andy-armstrong mentioned this pull request Mar 25, 2016

Upgrade Underscore.string #11963

Merged

11 tasks

jmbowman mentioned this pull request Jan 13, 2023

Make NPM post-processing agnostic to node_modules location #31519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with Underscore in the asset pipeline #11938

Fix issues with Underscore in the asset pipeline #11938

andy-armstrong commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

dan-f Mar 24, 2016

andy-armstrong Mar 24, 2016

dan-f commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

feanil Mar 24, 2016

andy-armstrong Mar 24, 2016

dan-f commented Mar 24, 2016

feanil Mar 24, 2016

andy-armstrong Mar 24, 2016

dan-f commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

cahrens Mar 24, 2016

cahrens commented Mar 24, 2016

andy-armstrong commented Mar 25, 2016

cahrens commented Mar 25, 2016

benpatterson Mar 25, 2016

benpatterson Mar 25, 2016

benpatterson commented Mar 25, 2016

andy-armstrong commented Mar 25, 2016

andy-armstrong commented Mar 25, 2016

feanil commented Mar 25, 2016

andy-armstrong commented Mar 25, 2016

dan-f commented Mar 25, 2016

benpatterson commented Mar 25, 2016

benpatterson commented Mar 25, 2016

Fix issues with Underscore in the asset pipeline #11938

Fix issues with Underscore in the asset pipeline #11938

Conversation

andy-armstrong commented Mar 24, 2016

Sandbox

Testing

Reviewers

Post-review

andy-armstrong commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Mar 24, 2016

andy-armstrong commented Mar 24, 2016

Choose a reason for hiding this comment

cahrens commented Mar 24, 2016

andy-armstrong commented Mar 25, 2016

cahrens commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpatterson commented Mar 25, 2016

andy-armstrong commented Mar 25, 2016

andy-armstrong commented Mar 25, 2016

feanil commented Mar 25, 2016

andy-armstrong commented Mar 25, 2016

dan-f commented Mar 25, 2016

benpatterson commented Mar 25, 2016

benpatterson commented Mar 25, 2016