Problem/Motivation

In preparation to being moved to core, let's fix violations against core eslint rules.

Steps for testing

  1. Apply patch from #3227826-2: Preparation for moving CKEditor 5 to core
  2. Clone this module under core/modules/ckeditor5
  3. Run yarn lint:core-js-passing in core directory and ensure there are no errors.
CommentFileSizeAuthor
#7 interdiff.txt545 byteslauriii
#7 3227848-7.patch434.62 KBlauriii
#6 interdiff.txt3.5 KBlauriii
#6 3227848-6.patch434.49 KBlauriii
#5 3227848-5.patch434.01 KBlauriii
#4 3227848-3.patch433.09 KBlauriii
#3 interdiff.txt22.3 KBlauriii
#3 3227848-3.patch0 byteslauriii
#2 3227848-2.patch414.3 KBlauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Active » Needs review
FileSize
3227848-2.patch414.3 KB

Here's a first pass. Let's see what the bot says about this.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3227848-3.patch0 bytes
interdiff.txt22.3 KB

Disabled cspell from some of the generated files.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3227848-3.patch433.09 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3227848-5.patch434.01 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3227848-6.patch434.49 KB
interdiff.txt3.5 KB

🙄

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3227848-7.patch434.62 KB
interdiff.txt545 bytes

😓

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Green!

But … why is the yarn.lock file changing so drastically 🤔🤯

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

How much effort should we put into researching the root cause for that? We haven't updated the yarn.lock since late June and there must be some updates to dependencies of our dependencies that cause these changes. Most of these changes happen automatically as a result of running yarn install in js/admin.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Not deep! Just wanted to understand this shockingly large diff.

IOW: you literally only did these things from the IS?

  1. Apply patch from #3227826-2: Preparation for moving CKEditor 5 to core
  2. Clone this module under core/modules/ckeditor5
  3. Run yarn lint:core-js-passing in core directory and ensure there are no errors.
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Did a bit of digging and it seems like most of the changes are caused by something changing in yarn, which is changing the order of the keys in yarn.lock.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Most of the fixes are coming from running yarn lint:core-js-passing --fix. However, there were manual changes required to make yarn lint:core-js-passing pass because the eslint violation fixer doesn't fix violations that have an impact on code logic. That said, all changes in the patch are required to either make yarn lint:core-js-passing or the CI cspell pass.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks for #11 and #12 — both of those comments really help make me feel more comfortable with this overwhelming diff 😄

Manually skimmed everything. Nothing stands out. Combined with #11+#12, I think it's time to just 🚢this.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Fixed

  • Wim Leers committed 2348d35 on 1.0.x authored by lauriii 
    Issue #3227848 by lauriii, Wim Leers: Fix violations against core eslint...

  • Wim Leers committed 5ce61c1 on 1.0.x authored by lauriii 
    Issue #3228338 by lauriii, Wim Leers, nod_: Follow-up for #3227848:...

Status: Fixed » Closed (fixed)

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