Problem/Motivation

Update stylelint to the latest version and fix a single CSS regression.

Needed so no warnings when installing stylelint-order - see #3024527: Add and configure stylelint-order

Proposed resolution

Do it.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Stylelint has been updated from 9.1.1 to 9.10.1

CommentFileSizeAuthor
#8 3038562-2-8.patch34.4 KBalexpott
#8 2-8-interdiff.txt27.28 KBalexpott
#6 3038562-4.patch133.92 KBmarkconroy
#2 3038562-2.patch52.67 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
FileSize
3038562-2.patch52.67 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Active » Needs review

The yarn.lock was updated by doing yarn upgrade stylelint

Status: Needs review » Needs work

The last submitted patch, 2: 3038562-2.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

markconroy’s picture

markconroy CreditAttribution: markconroy as a volunteer and at Annertech commented
Status: Needs work » Reviewed & tested by the community
FileSize
3038562-4.patch133.92 KB

Hi @alexpott

This looks good to me. I wonder would it be worth increasing the version in package.json to 9.10.1 to reflect this, or are we okay just having it in the yarn.lock file?

Just in case, here's a patch with the package.json updated and the yarn run lint:css --fix ran, with the same results.

I'll mark this RTBC in either case.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

@markconroy something else has happened on your yarn update - a load of integrity stuff has changed. Making interdiffs and checking what's happening helps. Maybe you are not on the latest yarn or something - I'm on 1.13.0

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
FileSize
2-8-interdiff.txt27.28 KB
3038562-2-8.patch34.4 KB 
yarn upgrade stylelint@^9.10.1
yarn upgrade v1.13.0
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
[5/5] 🔨  Rebuilding all packages...
success Saved lockfile.
success Saved 19 new dependencies.
info Direct dependencies
└─ stylelint@9.10.1
info All dependencies
├─ @babel/core@7.3.4
├─ @types/events@3.0.0
├─ @types/glob@7.1.1
├─ @types/minimatch@3.0.3
├─ @types/vfile-message@1.0.1
├─ @types/vfile@3.0.2
├─ dir-glob@2.2.2
├─ flatted@2.0.0
├─ global-modules@2.0.0
├─ global-prefix@3.0.0
├─ globby@9.1.0
├─ htmlparser2@3.10.1
├─ known-css-properties@0.11.0
├─ remark-parse@6.0.3
├─ remark-stringify@6.0.4
├─ remark@10.0.1
├─ stylelint@9.10.1
├─ unified@7.1.0
└─ vfile@3.0.1
✨  Done in 5.17s.

For some reason it's update less again /shrug javascript package management.

  • lauriii committed c73c894 on 8.8.x 
    Issue #3038562 by alexpott, markconroy: Update stylelint to 9.10.1

  • lauriii committed 34845b8 on 8.7.x 
    Issue #3038562 by alexpott, markconroy: Update stylelint to 9.10.1

(...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Confirmed manually that updating stylelint doesn't cause any new coding standard violations.

Committed c73c894 and pushed to 8.8.x. Patch- and minor-level library updates are allowed to be backported before RC phase, so I backported this to 8.7.x as well. Thanks!

  • xjm committed 69695d9 on 8.7.x 
    Revert "Issue #3038562 by alexpott, markconroy: Update stylelint to 9.10...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Fixed » Reviewed & tested by the community

We're still in commit freeze, so reverted from 8.7.x for now.

  • lauriii committed 7160424 on 8.7.x 
    Issue #3038562 by alexpott, markconroy: Update stylelint to 9.10.1

(...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Cherry-picked again now that the commit freeze is over. 😇

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +8.7.0 release notes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs change record

Missing a CR I think.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Issue summary: View changes

Do we really need a CR? This is a JS dev dependency and it's not a major version update? Also it doesn't affect the rules or how they are run at all. I.e. the change record has no change to report. The CSS change was due to a patch being committed that didn't comply with the current ruleset.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

We generally add CRs for all minor dependency updates AFAIK. Just because we don't know of any disruptive changes, that doesn't mean someone downstream using the library we provide isn't impacted.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Or in other terms, not having CRs for dependency updates causes extra work to find the updates, so let's just add CRs each time. We add CRs for way less impactful things.

Status: Fixed » Closed (fixed)

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