Use package.json to manage third party JS libraries

Looks good!

drupal/core on  3219088-use-package.json-and [!] via ⬢ v15.11.0 via 🐘 v7.4.3 took 32s 
❯ yarn vendor-update
yarn run v1.22.10
$ node ./scripts/js/assets.js
Copy backbone/backbone.js to backbone/backbone.js
Copy backbone/backbone-min.js to backbone/backbone-min.js
Process map file backbone-min.map
Copy css.escape/css.escape.js to css-escape/css.escape.js
Copy es6-promise/dist/es6-promise.auto.min.js to es6-promise/es6-promise.auto.min.js
Process map file dist/es6-promise.auto.min.map
Copy farbtastic/marker.png to farbtastic/marker.png
Copy farbtastic/mask.png to farbtastic/mask.png
Copy farbtastic/wheel.png to farbtastic/wheel.png
Copy farbtastic/farbtastic.css to farbtastic/farbtastic.css
Copy farbtastic/farbtastic.min.js to farbtastic/farbtastic.js
Copy jquery/dist/jquery.js to jquery/jquery.js
Copy jquery/dist/jquery.min.js to jquery/jquery.min.js
Process map file dist/jquery.min.map
Copy jquery-form/dist/jquery.form.min.js to jquery-form/jquery.form.min.js
Process map file dist/jquery.form.min.js.map
Copy jquery-form/src/jquery.form.js to jquery-form/src/jquery.form.js
Copy joyride/jquery.joyride-2.1.js to jquery-joyride/jquery.joyride-2.1.js
Copy jquery-once/jquery.once.js to jquery-once/jquery.once.js
Copy jquery-once/jquery.once.min.js to jquery-once/jquery.once.min.js
Process map file jquery.once.min.js.map
Copy js-cookie/dist/js.cookie.min.js to js-cookie/js.cookie.min.js
Copy normalize.css/normalize.css to normalize-css/normalize.css
Copy @drupal/once/dist/once.js to once/once.js
Copy @drupal/once/dist/once.min.js to once/once.min.js
Process map file dist/once.min.js.map
Copy picturefill/dist/picturefill.min.js to picturefill/picturefill.min.js
Copy @popperjs/core/dist/umd/popper.min.js to popperjs/popper.min.js
Process map file dist/umd/popper.min.js.map
Copy shepherd.js/dist/js/shepherd.min.js to shepherd/shepherd.min.js
Process map file dist/js/shepherd.min.js.map
Copy sortablejs/Sortable.min.js to sortable/Sortable.min.js
Copy tabbable/dist/index.umd.min.js to tabbable/index.umd.min.js
Process map file dist/index.umd.min.js.map
Copy underscore/underscore-min.js to underscore/underscore-min.js
Process map file underscore-min.js.map
Done in 0.14s.

Should our runtime dependencies be listed as true dependencies rather than dev dependencies? That would also help keep them separated from our build dependencies.

I don't think the linked discussion quite answers my question - if these are dev dependencies, what would be a production dependency in the case of core? package.json is in the core directory so I assumed it was only intended for core developers; if you are using JavaScript elsewhere in a Drupal site you would have a separate package.json in your theme or wherever.

When I using Node.js 14 LTS, file copying works successfully.
But, when I using Node.js 12 LTS, fails file copying because of undefined 'fs/promises' calling.
According to Node.js documentation, fs/promises is defined after Node.js 14.
I think below code will work on Node.js 12 and 14.

const { copyFile, writeFile, readFile, chmod } = require('fs').promises;

This looks really really neat. So nice to not have to maintain these manually!

Here are some notes I made while reviewing the patch - all positives

• Note: core/assets/vendor/jquery-form/src/jquery.form.js is added for the source map
• Note: core/assets/vendor/farbtastic/ JS and CSS changes are whitespace and comments
• This patch fixes the core/assets/vendor/jquery-form/jquery.form.min.js.map because the src is now pointed to a real source.
• This patch fixes core/assets/vendor/jquery/jquery.min.map to point to a valid source.

Here are two questions I have from reviewing the patch:

• Do we care that core/assets/vendor/farbtastic/marker.png is becoming less optimised - it's going from 437B to 652B? The other images are the same size at 11K and 2.0K - so maybe not?
• core/assets/vendor/popperjs/popper.min.js.map is changes to pointing from ../../src/dom-utils/getBoundingClientRect.js to src/dom-utils/getBoundingClientRect.js etc... neither file exists so this is not a regression but I don't think it is going to work. Same with the map for Shepherd and Tabbable. Maybe we should file a follow issue? Or is this expected? Or given we're manipulating the map files should we only do this when we're also providing the source - as in jquery and jquery.form? Or should we also be copying in the sources for these too?

The questions were answered, no changes required that I can see. This looks like a neat solution to our JS dependency management, so let's get this RTBC.

This looks really good to me, it's only a change for core developers, so if we run into problems later with it we can try to come up with a new system, but hopefully it'll be a good improvement with no drawbacks.

I haven't reviewed this in detail yet but I like the overall approach. Do you think it would make sense to open a follow-up for adding a CI check that ensures that the vendor files are indeed generated by the tool? I think it's fine as is for the time being but it would help reduce some work since it's a step I always do when committing minified files.

Catch, Alex and I have reviewed this now - removing the tag

I've pushed some minor nit-picks to the branch.

This looks good to me, I think just a follow-up for #30 is all that remains.

Ah, I didn't realise it was a future reserved word, reverted that change and expanded the comment to explain why we use pack

Should have removed the RM review tag with #29.

Committed to 9.3.x, thanks all

Published change record

npm install does not work any more after this MR landed. See: #3226441: npm install does not work for farbtastic and joyride with code being hard linked.