Problem/Motivation

yarn audit shows the following numbers of vulnerabilities on each branch:

  • 10.0.x, 9.4.x, and 9.3.x: 18 Moderate | 3 High
  • 9.2.x: 24 Moderate | 4 High

Steps to reproduce

  1. cd core
  2. yarn install
  3. yarn audit

Proposed resolution

Not all of the dependency updates can be addressed without increasing our constraints. However, running a yarn upgrade without modifying the constraints gets rid of some of the vulnerabilities, reducing it to:

  • 10.0.x, 9.4.x, 9.3.x: 16 Moderate | 1 High
  • 9.2.x: 12 Moderate

Let's fix those first (after reviewing that the upgraded versions are safe and testing). Then, let's handle whatever increases we need to make to constraints in followup issues.

Remaining tasks

Address remaining vulnerabilities that require changes to constraints in followup issues.

Followup issue for the acorn/terser dependency workaround: #3264520: Remove acorn from package.json

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Release notes snippet

Drupal core's JavaScript development dependencies have been updated to the latest allowed minor and patch versions to address a few security issues in those dependencies. This should have minimal impact on contributed or custom code and CI workflows. Core developers should completely remove their node_modules directory and re-run yarn install from within the core/ directory.

Additionally, Acorn has temporarily been added as a direct development dependency of core to work around an upstream bug in Terser. Acorn will be removed as a direct dependency again once Terser creates a new release with a fix for the bug.

Issue fork drupal-3262573

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -9.3.5 release notes, -9.2.12 release notes +9.3.6 release notes, +9.2.13 release notes
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.2.x-dev » 10.0.x-dev

Starting with the 10.0.x version MR...

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Maybe a duplicate of
#3258933: [meta] Update JavaScript dependencies for Drupal 10
#3246148: [meta] Update JavaScript dev dependencies to latest major releases before 9.4.0

xjm opened merge request !1780

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review

Not quite a duplicate; the scope in this issue is only ugprades within current constraints (which fixes a few vulns). We're also going to be backporting this all the way to at least 9.3.x and possibly 9.2.x.

Those issues are logical followups though. @lauriii will also be posting individual issues for dealing with a couple of specific dependencies constraining us to insecure versions.

Here's the yarn-lock-diff for the MR for 10.0.x:

package name old version(s) new version(s)
@babel/code-frame [..., 7.16.7], 7.15.8 [...], 7.16.7
@babel/compat-data 7.15.0 7.17.0
@babel/core 7.15.8 7.17.0
@babel/generator 7.15.8 7.17.0
@babel/helper-annotate-as-p 7.15.4 7.16.7
@babel/helper-builder-binar 7.15.4 7.16.7
@babel/helper-compilation-t 7.15.4 7.16.7
@babel/helper-create-class 7.15.4 7.17.1
@babel/helper-create-regexp 7.14.5 7.17.0
@babel/helper-define-polyfi 0.2.3 0.3.1
@babel/helper-explode-assig 7.15.4 7.16.7
@babel/helper-function-name 7.15.4 7.16.7
@babel/helper-get-function 7.15.4 7.16.7
@babel/helper-hoist-variabl 7.15.4 7.16.7
@babel/helper-member-expres 7.15.4 7.16.7
@babel/helper-module-imports 7.15.4 7.16.7
@babel/helper-module-transf 7.15.8 7.16.7
@babel/helper-optimise-call 7.15.4 7.16.7
@babel/helper-plugin-utils 7.14.5 7.16.7
@babel/helper-remap-async-t 7.15.4 7.16.8
@babel/helper-replace-supers 7.15.4 7.16.7
@babel/helper-simple-access 7.15.4 7.16.7
@babel/helper-skip-transpar 7.15.4 7.16.0
@babel/helper-split-export 7.15.4 7.16.7
@babel/helper-validator-ide [..., 7.16.7], 7.15.7 [...], 7.16.7
@babel/helper-validator-opt 7.14.5 7.16.7
@babel/helper-wrap-function 7.15.4 7.16.8
@babel/helpers 7.15.4 7.17.0
@babel/highlight [..., 7.16.10], 7.14.5 [...], 7.16.10
@babel/parser 7.15.8 7.17.0
@babel/plugin-bugfix-v8-spr 7.15.4 7.16.7
@babel/plugin-proposal-asyn 7.15.8 7.16.8
@babel/plugin-proposal-clas 7.14.5 7.16.7
@babel/plugin-proposal-clas 7.15.4 7.16.7
@babel/plugin-proposal-dyna 7.14.5 7.16.7
@babel/plugin-proposal-expo 7.14.5 7.16.7
@babel/plugin-proposal-json 7.14.5 7.16.7
@babel/plugin-proposal-logi 7.14.5 7.16.7
@babel/plugin-proposal-null 7.14.5 7.16.7
@babel/plugin-proposal-nume 7.14.5 7.16.7
@babel/plugin-proposal-obje 7.15.6 7.16.7
@babel/plugin-proposal-opti 7.14.5 7.16.7
@babel/plugin-proposal-opti 7.14.5 7.16.7
@babel/plugin-proposal-priv 7.14.5 7.16.11
@babel/plugin-proposal-priv 7.15.4 7.16.7
@babel/plugin-proposal-unic 7.14.5 7.16.7
@babel/plugin-transform-arr 7.14.5 7.16.7
@babel/plugin-transform-asy 7.14.5 7.16.8
@babel/plugin-transform-blo 7.14.5 7.16.7
@babel/plugin-transform-blo 7.15.3 7.16.7
@babel/plugin-transform-cla 7.15.4 7.16.7
@babel/plugin-transform-com 7.14.5 7.16.7
@babel/plugin-transform-des 7.14.7 7.16.7
@babel/plugin-transform-dot 7.14.5 7.16.7
@babel/plugin-transform-dup 7.14.5 7.16.7
@babel/plugin-transform-exp 7.14.5 7.16.7
@babel/plugin-transform-for 7.15.4 7.16.7
@babel/plugin-transform-fun 7.14.5 7.16.7
@babel/plugin-transform-lit 7.14.5 7.16.7
@babel/plugin-transform-mem 7.14.5 7.16.7
@babel/plugin-transform-mod 7.14.5 7.16.7
@babel/plugin-transform-mod 7.15.4 7.16.8
@babel/plugin-transform-mod 7.15.4 7.16.7
@babel/plugin-transform-mod 7.14.5 7.16.7
@babel/plugin-transform-nam 7.14.9 7.16.8
@babel/plugin-transform-new 7.14.5 7.16.7
@babel/plugin-transform-obj 7.14.5 7.16.7
@babel/plugin-transform-par 7.15.4 7.16.7
@babel/plugin-transform-pro 7.14.5 7.16.7
@babel/plugin-transform-reg 7.14.5 7.16.7
@babel/plugin-transform-res 7.14.5 7.16.7
@babel/plugin-transform-sho 7.14.5 7.16.7
@babel/plugin-transform-spr 7.15.8 7.16.7
@babel/plugin-transform-sti 7.14.5 7.16.7
@babel/plugin-transform-tem 7.14.5 7.16.7
@babel/plugin-transform-typ 7.14.5 7.16.7
@babel/plugin-transform-uni 7.14.5 7.16.7
@babel/plugin-transform-uni 7.14.5 7.16.7
@babel/preset-env 7.15.8 7.16.11
@babel/register 7.15.3 7.17.0
@babel/runtime-corejs3 7.15.4 7.17.0
@babel/runtime 7.15.4 7.17.0
@babel/template 7.15.4 7.16.7
@babel/traverse 7.15.4 7.17.0
@babel/types 7.15.6 7.17.0
@cspell/cspell-bundled-dicts 5.12.3 5.18.3
@cspell/cspell-types 5.12.3 5.18.3
@cspell/dict-bash 1.0.16 1.0.18
@cspell/dict-companies 1.0.40 2.0.2
@cspell/dict-csharp 1.0.11 2.0.1
@cspell/dict-css 1.0.12 1.0.13
@cspell/dict-en_us 2.1.2 2.1.5
@cspell/dict-filetypes 1.1.8 2.0.1
@cspell/dict-fullstack 1.0.39 2.0.4
@cspell/dict-html 1.1.9 2.0.3
@cspell/dict-php 1.0.24 1.0.25
@cspell/dict-public-licenses 1.0.3 1.0.4
@cspell/dict-python 2.0.4 2.0.5
@cspell/dict-ruby 1.0.14 1.0.15
@cspell/dict-software-terms 1.0.48 2.0.14
@cspell/dict-typescript 1.0.19 1.0.20
@discoveryjs/json-ext 0.5.5 0.5.6
@humanwhocodes/object-schema 1.2.0 1.2.1
@npmcli/fs 1.0.0 1.1.0
@popperjs/core 2.10.2 2.11.2
@testim/chrome-version 1.0.7 1.1.2
@types/eslint-scope 3.7.1 3.7.3
@types/eslint 7.28.2 8.4.1
@types/node 16.11.6 17.0.14
@webpack-cli/configtest 1.1.0 1.1.1
@webpack-cli/info 1.4.0 1.4.1
@webpack-cli/serve 1.6.0 1.6.1
acorn 8.5.0 8.7.0
ajv 8.9.0 8.10.0
ansi-regex [..., 6.0.1
assertion-error [..., 1.1.0], 1.0.0 [...], 1.1.0
axe-core 4.3.4 4.4.1
babel-plugin-polyfill-corej 0.2.2 0.3.1
babel-plugin-polyfill-corej 0.2.5 0.5.2
babel-plugin-polyfill-regen 0.2.2 0.3.1
browserslist [..., 4.17.6], 4.17.5 [...], 4.19.1
bytes 3.1.0 3.1.1
caniuse-lite [..., 1.0.30001279], 1.0.30001271 [...], 1.0.30001307
chai-nightwatch 0.4.1 0.4.2
chokidar 3.5.2 3.5.3
clear-module 4.1.1 4.1.2
color-string 1.6.0 1.9.0
commander 8.3.0 9.0.0
comment-json 4.1.1 4.2.2
confusing-browser-globals 1.0.10 1.0.11
core-js-compat 3.19.0 3.21.0
core-js-pure 3.19.0 3.21.0
cspell-gitignore 5.12.3 5.18.3
cspell-glob 5.12.3 5.18.3
cspell-io 5.12.3 5.18.3
cspell-lib 5.12.3 5.18.3
cspell-trie-lib 5.12.3 5.18.3
cspell 5.12.3 5.18.3
damerau-levenshtein 1.0.7 1.0.8
debug [..., 4.3.3], 4.3.2 [...], 4.3.3
deep-eql 0.1.3
electron-to-chromium [..., 1.3.893], 1.3.880 [...], 1.4.64
eslint-module-utils 2.7.1 2.7.3
eslint-plugin-import 2.25.2 2.25.4
eslint-plugin-jsx-a11y 6.4.1 6.5.1
eslint-plugin-react-hooks 4.2.0 4.3.0
eslint-plugin-react 7.26.1 7.28.0
extsprintf 1.4.0 1.4.1
fast-glob [..., 3.2.7
flatted 3.2.4 3.2.5
follow-redirects 1.14.4 1.14.7
globals 13.12.0 13.12.1
globby [..., 8.0.2], 11.1.0, 11.0.4 [...], 8.0.2, 11.1.0
graceful-fs 4.2.8 4.2.9
http-errors 1.7.3 1.8.1
ignore [..., 5.2.0], 5.1.9 [...], 5.2.0
import-local 3.0.3 3.1.0
is-core-module [..., 2.8.1], 2.8.0 [...], 2.8.1
is-negative-zero 2.0.1 2.0.2
is-weakref 1.0.1 1.0.2
jest-worker 27.3.1 27.4.6
jsdom 18.0.1 18.1.1
json-schema 0.2.3 0.4.0
jsprim 1.4.1 1.4.2
lodash.clonedeep 4.5.0
mime-db [..., 1.51.0], 1.50.0 [...], 1.51.0
mime-types [..., 2.1.34], 2.1.33 [...], 2.1.34
mime 2.5.2 2.6.0
minipass 3.1.5 3.1.6
nightwatch 1.7.11 1.7.13
node-modules-regexp 1.0.0
object-inspect 1.11.0 1.12.0
picomatch [..., 2.3.1], 2.3.0 [...], 2.3.1
pirates 4.0.1 4.0.5
pkg-dir [..., 4.2.0], 3.0.0, 2.0.0 [...], 4.2.0, 3.0.0
pofile 1.1.1 1.1.3
postcss-selector-parser [..., 6.0.9], 6.0.6 [...], 6.0.9
postcss-value-parser [..., 4.2.0], 4.1.0 [...], 4.2.0
postcss 8.4.5 8.4.6
prettier 2.4.1 2.5.1
prop-types 15.7.2 15.8.1
raw-body 2.4.1 2.4.2
regenerate-unicode-properti 9.0.0 10.0.1
regexp.prototype.flags 1.3.1 1.4.1
regexpu-core 4.8.0 5.0.1
regjsgen 0.5.2 0.6.0
regjsparser 0.7.0 0.8.4
resolve [..., 2.0.0-next.3], 1.21.1, 1.20.0 [...], 2.0.0-next.3, 1.22.0
setprototypeof 1.1.1 1.2.0
shelljs 0.8.4 0.8.5
signal-exit [..., 3.0.6], 3.0.5 [...], 3.0.7
source-map-support 0.5.20 0.5.21
sshpk 1.16.1 1.17.0
stylelint 14.2.0 14.3.0
table [..., 6.8.0], 6.7.2 [...], 6.8.0
terser-webpack-plugin 5.2.5 5.3.1
terser 5.9.0 5.10.0
toidentifier 1.0.0 1.0.1
tsconfig-paths 3.11.0 3.12.0
type-detect 0.1.1
typedarray-to-buffer [..., 4.0.0
underscore 1.13.1 1.13.2
vscode-uri 3.0.2 3.0.3
watchpack 2.2.0 2.3.1
webpack-cli 4.9.1 4.9.2
webpack-sources 3.2.1 3.2.3
webpack 5.63.0 5.68.0
write-file-atomic [..., 4.0.0
ws 8.2.3 8.4.2
@ampproject/remapping 2.0.2
@babel/helper-environment-v 7.16.7
@babel/plugin-bugfix-safari 7.16.7
@cspell/cspell-pipe 5.18.3
@cspell/dict-r 1.0.1
@cspell/dict-swift 1.0.1
@cspell/dict-vue 2.0.1
@jridgewell/resolve-uri 3.0.4
@jridgewell/trace-mapping 0.2.6
fast-equals 2.0.4
sourcemap-codec 1.4.8
supports-hyperlinks 2.2.0
xjm’s picture

xjm
she/her
Primary language English
commented
Title: Update our yarn dev dependencies » Update our yarn dev dependencies to the extent allowed by current constraints
xjm’s picture

xjm
she/her
Primary language English
commented

The MR is failing with:

--- Commands Executed ---
core/scripts/dev/commit-code-check.sh --drupalci
Return Code: 1
--- Output ---
Creating list of files to check by comparing branch to 10.0.x
Drupal's JavaScript development dependencies are not installed. Run 'yarn install' inside the core directory.
--- Errors ---

...which, like. I have to do that on the CLI all the time, but I don't know how to make Testbot do that.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

On mobile so can't check for sure but it seems likely this is the same issue as @alexpott had in #3258933: [meta] Update JavaScript dependencies for Drupal 10 as there doesn't seem to have been a new release of terser yet, which has some dependency warning with acorn.

xjm’s picture

xjm
she/her
Primary language English
commented

I confirmed that I can reproduce this with the MR:

[ayrton:core | Fri 17:31:33] $ yarn check -s
warning ../../../package.json: No license field
error "terser#acorn@^8.5.0" doesn't satisfy found match of "acorn@7.4.1"
warning "eslint#@babel/code-frame#@babel/highlight@^7.10.4" could be deduped from "7.16.10" to "@babel/highlight@7.16.10"
error Found 1 errors.

If I run yarn check -s on 10.0.x HEAD, it outputs even more stuff:

[ayrton:core | Fri 17:29:13] $ yarn check -s
warning ../../../package.json: No license field
warning "eslint-plugin-import#is-core-module@^2.7.0" could be deduped from "2.8.1" to "is-core-module@2.8.1"
warning "postcss-preset-env#browserslist@^4.6.4" could be deduped from "4.17.6" to "browserslist@4.17.6"
warning "eslint#@babel/code-frame#@babel/highlight@^7.10.4" could be deduped from "7.16.10" to "@babel/highlight@7.16.10"
warning "postcss-preset-env#autoprefixer#browserslist@^4.12.0" could be deduped from "4.17.6" to "browserslist@4.17.6"
warning "postcss-preset-env#browserslist#electron-to-chromium@^1.3.878" could be deduped from "1.3.893" to "electron-to-chromium@1.3.893"
warning "@babel/preset-env#core-js-compat#browserslist@^4.17.5" could be deduped from "4.17.6" to "browserslist@4.17.6"
warning "chromedriver#del#globby@^11.0.1" could be deduped from "11.1.0" to "globby@11.1.0"
warning "webpack-cli#execa#signal-exit@^3.0.3" could be deduped from "3.0.6" to "signal-exit@3.0.6"
warning "@babel/core#@babel/helper-compilation-targets#browserslist@^4.16.6" could be deduped from "4.17.6" to "browserslist@4.17.6"
warning "autoprefixer#browserslist#electron-to-chromium@^1.3.878" could be deduped from "1.3.893" to "electron-to-chromium@1.3.893"
warning "@babel/helper-compilation-targets#browserslist#electron-to-chromium@^1.3.878" could be deduped from "1.3.893" to "electron-to-chromium@1.3.893"
warning "core-js-compat#browserslist#electron-to-chromium@^1.3.878" could be deduped from "1.3.893" to "electron-to-chromium@1.3.893"
warning "del#globby#fast-glob@^3.0.3" could be deduped from "3.2.11" to "fast-glob@3.2.11"
warning "del#globby#ignore@^5.1.1" could be deduped from "5.2.0" to "ignore@5.2.0"

... But I guess those are all warnings and not errors.

If I run:
core/scripts/dev/commit-code-check.sh --drupalci

...it passes on 10.0.x, but says this on the MR branch:

[ayrton:drupal | Fri 17:32:33] $ core/scripts/dev/commit-code-check.sh --drupalci
Creating list of files to check by comparing branch to 10.0.x
Drupal's JavaScript development dependencies are not installed. Run 'yarn install' inside the core directory.

I guess it's just very catch-all error handling? The yarn build failed, and whatever the reason it failed, it just assumes that it's because yarn install was not run?

...Yep, that is exactly what's happening:

# Ensure JavaScript development dependencies are installed.                     
yarn check -s 2>/dev/null
if [ "$?" -ne "0" ]; then
  printf "Drupal's JavaScript development dependencies are not installed. Run 'yarn install' inside the core directory.\n"
  DEPENDENCIES_NEED_INSTALLING=1;
fi

So if that upstream issue is what will fix it, are we forced to wait on them to make an upstream release in order to resolve the shelljs vuln etc.? (As far as I can tell, yarn lacks anything like Composer's --with-dependencies, so I can never get anything to upgrade unless I hit it with the firehose of a full yarn upgrade.)

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Adding the acorn issue to the IS.

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#3238497: What to do with assets build step?

I was also surprised by just how many files were changed by rerunning yarn build after these upgrades. However, it turns out that the changes also happen on 10.0.x HEAD! My hypothesis is that it's because we dropped support for IE11, but have not yet implemented #3238497: What to do with assets build step?.

Of course, that won't apply to 9.4.x, 9.3.x, and 9.2.x, all of which also need to be fixed. And we wouldn't want to commit the updates to 9.4.x before 10.0.x, because that would violate the backport policy... I don't want to block fixing those security vulns on removing the build step, either.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Adding acorn as an explicit dependency seems to solve the issue, we should be able to remove it again when a new version of terser is released.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think the JS changes noted in #13 are due to a bug fix in Babel 7.16, possibly https://github.com/babel/babel/pull/13842 - the input/output in the tests there look similar to the changes here where destructuring is rewritten to go through _ref, and if I take Safari and iOS out of our browser support list, the changes go away.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

An update to Stylelint also caused a new error: Expected "currentColor" to be "currentcolor" value-keyword-case

Solved with "camelCaseSvgKeywords": true as per https://github.com/stylelint/stylelint/issues/5863

longwave opened merge request !1781

longwave opened merge request !1782

longwave opened merge request !1783

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I've opened the following issue on github to ask terser for a release - https://github.com/terser/terser/issues/1143 - as I think they've already fixed the issue.

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @longwave. Looks like you're right about the Babel bugfix; I must've had a stale node_modules and forgotten to yarn install again after switching branches. Can't reproduce it now.

Adding an explicit dependency until there's an upstream release is a great workaround; +1.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

This looks great. Nice work on explaining why the changes are occurring and keeping the CSS the same.

nod_’s picture

nod_
Primary language French
Location Lille
commented

+1 for RTBC

Changes to the JS are mostly whitespace. Setting the prototype as unwrittable for Drupal.Message and Drupal.DatepickerPolyfill doesn't impact contrib (that I can see) so the change is safe.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Do we need dependency evaluation for acorn given that it's now a direct dev dependency?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Upgrading CSpell means we need to rebuild the dictionary for all branches.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Dictionaries updated.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

> Do we need dependency evaluation for acorn given that it's now a direct dev dependency?

I would suggest not, as it was an indirect dependency before, we don't use it directly, and this is hopefully only temporary - once the next version of terser is released we should be able to remove it again.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs followup

Yeah, in lieu of a dep evaluation, let's file a followup to remove the dep again when upstream is updated.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Babel release a new patch release last week https://github.com/babel/babel/releases/tag/v7.17.2 so MRs needs to be updated. There are a couple of other releases to other dependencies as well but nothing major. I updated the different MRs

Other than that I checked that the compiled js/css didn't change in any meaningful way and that all the commands in the package.json were still working.

9.2 : OK - yarn audit 12 Moderate vulnerabilities (all about regex)
9.3 : OK - yarn audit 16 Moderate | 1 High (same regex stuff as 9.2 + 5 more regex issues from ckeditor deps)
9.4 : OK - yarn audit 16 Moderate | 1 High (same as 9.3)
10.0 : OK - yarn audit 16 Moderate | 1 High (same as 9.3)

I pushed some code but this is RTBC for me.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

I opened #3264520: Remove acorn from package.json as a followup.

I checked each of @nod_'s "yarn upgrade" commits and they all look OK to me with minor/patch level versions only, so I am marking this RTBC alongside the comment above.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Updated the IS remaining tasks section with the followup issue.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Added a release note.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Merged in the head of each branch so the patches apply cleanly.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

On 10.0.x (and perhaps others) Running yarn vendor-update results in changes that should be part of the MR. Setting back to NW for this + rebase will likely be needed due to #3228464: API for contrib projects to load CKEditor translations.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Ran yarn vendor-update on 9.3.x, 9.4.x and 10.0.x (it doesn't exist in 9.2.x).

  • bnjmnm committed 8a0b901 on 10.0.x authored by xjm 
    Issue #3262573 by longwave, nod_, xjm, alexpott, lauriii: Update our...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Needs review » Reviewed & tested by the community

Thanks @longwave! With a small cspell adjustment due to a recent commit, this is good for 10.0.x. I'm committing via Gitlab merge and will continue reviewing for potential commit in descending order.

  • bnjmnm committed 4ab0486 on 9.4.x authored by longwave 
    Issue #3262573 by longwave, nod_, xjm, alexpott, lauriii: Update our...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Version: 9.4.x-dev » 9.3.x-dev

I made a similar tiny cspell adjustment, which makes 9.4.x fine to commit. On to 9.3!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@terser is going to get a release on Monday so we should be able to clean this bit of this up quite soon.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

We have #3264520: Remove acorn from package.json for what's already gone in, but it might be worth updating the 9.3.x and 9.2.x MRs before commit so they never have the direct dependency.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

#3264520: Remove acorn from package.json has landed so adding the acorn dependency is no longer necessary.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical

Bumping to critical. It would be good to get this in before the next 9.3 patch release.

Spokje made their first commit to this issue’s fork.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Tried _really_ hard, but doing a yarn remove acorn only resulted in the removal of acorn from package.json on both 9.3.x and 9.2.x.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Are we allowed to do a vendor-update in a patch release? This is end-user-facing JavaScript and here will bump the version of popperjs from 2.10.2 to 2.11.2, and underscore from 1.13.1 to 1.13.2.

spokje’s picture

spokje
Primary language Dutch
commented

As far as I'm aware I only did a rebase using the GitLab GUI and a yarn remove acorn as shown here.
I normally prefer merging the branch into the MR, but was told here #3264122-20: Move all non migration aggregator tests to the module in preparation of removal in d10 rebasing was preferred.

No clue where the updates come from or what I could have done wrong.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Sorry, #46 wasn't a response/question directly to you, more an observation that we upgraded these packages for 9.4.x/10.0.x, but can we safely do that in 9.3.x seeing as we already released 9.3.0 - or should we be more conservative if we aren't fixing security issues? We treat all JS deps as dev dependencies, but popper, underscore, etc are used in the front end of sites, they are only dev dependencies because they are part of our build process.

spokje’s picture

spokje
Primary language Dutch
commented

Ah, no problem, love questions that aren't (directly) directed at me ;)

xjm’s picture

xjm
she/her
Primary language English
commented

Rebase vs. merge is somewhat moot in an issue like this where we have to re-run build scripts anyway; I would do neither really. The acorn dependency was also never committed to 9.3.x nor 9.2.x, so we should just be able to recreate the MR cleanly without it. Carrying around a bunch of previous commits for it isn't really helpful in this case.

We should be restricting production vendor updates to patch versions in patch releases, but the scope of this issue was intended to include only the dev dependencies and not the vendor update. If the vendor update is happening as part of yarn run build:js, then that's something we need to file an issue about, I think.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs followup

Discussed this with @lauriii and he suggested that we add a constraint in this issue for Popper to keep it restricted to a patch-level update for 9.3.x, and then to re-run the yarn upgrade and the vendor build. (I think it doesn't matter for 9.2.x since the vendor update script wasn't in 9.2 anyway.)

Then, we'll file a followup to add constraints on the production dependencies so we can keep the vendor update consistent but locked to patch versions unless we make an explicit decision to do a minor update in a patch release. Going forward, each minor, we'll deliberately increase constraints to the latest minor version for the production vendor dependencies prior to beta/RC.

xjm’s picture

xjm
she/her
Primary language English
commented

We should probably expand the frontend tools dependency management docs to add more detailed instructions equivalent to the various sections of the core Composer dependency update instructions. There are four total scenarios:

  1. Updating production dependencies, including changing constraints or adding/removing deps.
  2. Updating production dependencies within current constraints (needs a followup to constrain them to patch versions).
  3. Updating dev dependencies including changing constraints or adding/removing deps.
  4. Updating dev dependencies without changing constraints.

I think it's something like:

  1. Check out HEAD of the applicable branch.
  2. Increase constraints in core/package.json (for production deps) or package.json (for dev deps) as appropriate.
  3. Delete the old node_modules folder.
  4. Run yarn install from within the core directory.
  5. Run yarn build.
  6. Run yarn vendor-update.
  7. If cspell has been updated, re-run yarn run spellcheck:make-drupal-dict.
xjm’s picture

xjm
she/her
Primary language English
commented

I force-pushed the 9.3.x MR with that pattern. Note that I explicitly set the Popper.js constraint to be less than the next minor:

@popperjs/core": "^2.9.2 <2.11.0",

...rather than using ~ or similar because this seems the least disruptive: The current constraint is ^2.9.2, but the locked/vendor version is 2.10.2. We can fix this in a better way in the proposed followup.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Opened #3266912: Review version constraints for production yarn dependencies to review version constraints for all production dependencies.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

MR is failing with:

yarn run v1.22.17
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /var/www/html/core/modules/ckeditor5/js/ckeditor5.admin.es6.js
[21:29:35] '/var/www/html/core/modules/ckeditor5/js/ckeditor5.admin.es6.js' is being checked.
Done in 1.18s.

/var/www/html/core/modules/ckeditor5/js/ckeditor5.admin.es6.js
  670:9  error  Expected indentation of 8 space characters but found 10  react/jsx-indent

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Looks like the 9.3.x MR still needs some work. There's a problem with the latest version of eslint-plugin-react since they introduced stricter validation constraints for React version configuration which is currently set to "latest" which isn't a valid semver version. We might have to pin that to 7.29.0 until we have that sorted out.

On top of that, there's some changes after running yarn build:js which seem incorrect. For some reason some files are not being built correctly because I can see that the built files have const, arrow functions and class syntax.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

#55 seems to be a false positive in eslint-plugin-react 7.29.1 and fixed in 7.29.2. Pushed an upgrade commit to the MR.

Also switched from latest to detect in the React version, which falls back to latest while emitting a warning that we can hopefully ignore for now.

I don't see the changes @lauriii mentions in #56, yarn build is clean for me, so back to NR for another look.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Same for me build is clean, probably something in the cache that messes things up? to be safe I delete the node_modules directory pretty frequently :)

Backbone has been bumped to 1.4.1 (from 1.4.0) since they had a release a few days ago. Either need to stay on 1.4 or run vendor-update.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Backbone updated to 1.4.1 in 9.3.x.

nod_’s picture

nod_
Primary language French
Location Lille
commented

let's keep it at 1.4.0 then, and create a new issue to update all the branches at the same time.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Pinned Backbone to 1.4.0, ran yarn vendor-update again.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Opened #3267339: Update Backbone to 1.4.1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Re #61 I think the test fail for backbone 1.4.1 is because we've not updated core/core.libraries.yml to have the correct version - I think to get 9.3.x tests passing on backbone 1.4.1 all we need to do is make similar changes like we're doing for underscore.

nod_’s picture

nod_
Primary language French
Location Lille
commented

The version is hardcoded in a test, backbone was chosen because it doesn't release new versions often i think, so we wouldn't need to update the test often.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yeah the test fail was this hardcoded in AttachedAssetsTest:

    $this->assertStringContainsString('core/assets/vendor/backbone/backbone-min.js?v=1.4.0', $rendered_js, 'JavaScript version identifiers correctly appended to URLs');

However IMO we should not upgrade Backbone on 9.3.x before we have done 9.4.x/10.0.x so I think deferring to a separate issue where it can be committed to all branches together is better.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

  • bnjmnm committed 48d3c58 on 9.3.x authored by longwave 
    Issue #3262573 by longwave, nod_, xjm, Spokje, alexpott, lauriii: Update...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Needs work

Committed to 9.3.x. 9.2.x currently "Custom commands failed".

Also needs followup per #51 before this can be marked fixed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

In the 9.2.x branch we are back to this problem:

$ yarn check -s
error "terser#acorn@^8.5.0" doesn't satisfy found match of "acorn@7.4.1"

Running yarn upgrade terser makes it worse somehow:

$ yarn check -s
error "acorn" is wrong version: expected "8.7.0", got "7.4.1"
error "espree#acorn" not installed
error "espree#acorn-jsx" not installed

Not sure what the next steps are here.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

From looking at the terser release notes it seems like it should be safe to lock terser into staying on 5.7.0 in Drupal 9.2 and not concern ourselves with dependency headaches on the version that will be unsupported soonest.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

terser 5.10.0 and above seem to cause the issue so I set package.json to ^5.3.4 <5.10.0 which has solved it locally for me.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

yarn audit returns 9 Moderate vulnerabilites on 9.2.x, there is a babel update but it doesn't change the number of vulnerabilites, so RTBC :)

Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented

Posted #3267705: Fix error message when 'yarn check -s' fails in the commit check script for the debug difficulty documented in #11.

  • bnjmnm committed f170243 on 9.2.x authored by longwave 
    Issue #3262573 by longwave, nod_, xjm, Spokje, alexpott, lauriii, catch...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Committed to 9.2.x. I can't mark this fixed until the followup requested in #51 is filed:

Discussed this with @lauriii and he suggested that we add a constraint in this issue for Popper to keep it restricted to a patch-level update for 9.3.x, and then to re-run the yarn upgrade and the vendor build. (I think it doesn't matter for 9.2.x since the vendor update script wasn't in 9.2 anyway.)

Then, we'll file a followup to add constraints on the production dependencies so we can keep the vendor update consistent but locked to patch versions unless we make an explicit decision to do a minor update in a patch release. Going forward, each minor, we'll deliberately increase constraints to the latest minor version for the production vendor dependencies prior to beta/RC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Wasn't the followup created in #54? #3266912: Review version constraints for production yarn dependencies

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

Re #76 @longwave you are correct! Looks like the tag just wasn't removed. Thanks and FIXED!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.