Automattic / newspack-theme

A theme for Newspack.
https://newspack.com
GNU General Public License v2.0
305 stars 66 forks source link

fix: update dependencies to avoid conflict in WP 6.6 #2318

Closed dkoo closed 3 months ago

dkoo commented 5 months ago

All Submissions:

Changes proposed in this Pull Request:

Refactoring required for changes in https://github.com/Automattic/newspack-scripts/pull/209. Note that this PR will not pass CI tests and should NOT be merged until https://github.com/Automattic/newspack-scripts/pull/209 is released as an NPM package, so this repo can install it as a dependency.

How to test the changes in this Pull Request:

Follow instructions in https://github.com/Automattic/newspack-scripts/pull/209.

Other information:

miguelpeixe commented 3 months ago

I find it strange that when I build/watch I get a bunch of deprecation warnings, e.g.:

    ╷
51  │ ┌     &,
52  │ │     &:visited:not(:hover) {
53  │ │         background: #d33;
54  │ │         color: #fff;
55  │ │     }
    │ └─── nested rule
... │
58  │       margin-left: auto;
    │       ^^^^^^^^^^^^^^^^^ declaration
    ╵
    newspack-theme/sass/navigation/_menu-mobile-navigation.scss 58:2  @use
    newspack-theme/sass/navigation/_navigation.scss 15:1              @use
    newspack-theme/sass/style-base.scss 22:1                          @use
    newspack-joseph/sass/style.scss 3:1                               root stylesheet

Deprecation Warning: Sass's behavior for declarations that appear after nested
rules will be changing to match the behavior specified by CSS in an upcoming
version. To keep the existing behavior, move the declaration above the nested
rule. To opt into the new behavior, wrap the declaration in `& {}`.

More info: https://sass-lang.com/d/mixed-decls

But those are not caught with npm run lint. Is that expected?

dkoo commented 3 months ago

@miguelpeixe I think this is because the Theme repo uniquely uses sass as a direct dev dependency, whereas the other repos just use whatever peer dependency @wordpress/scripts (and before that, Calypso Build) uses. Looks like the peer dependency specifies ^1.35.2 whereas the direct dependency in Theme is now ^1.77.8. Looks like the deprecation warning was just introduced in 1.77.7, so the linters from @wordpress/scripts won't show it since they're running an older version.

If it's obtrusive, we can always silence the warnings in our Theme config. I'm fairly certain SASS wouldn't introduce a breaking change like this in a minor version, so the risk would be low.

matticbot commented 3 months ago

:tada: This PR is included in version 2.0.0-alpha.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

matticbot commented 3 months ago

:tada: This PR is included in version 2.0.0-epic-ras-acc.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

matticbot commented 3 months ago

:tada: This PR is included in version 2.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: