Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Aug 2, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jun 14, 2024

All Submissions:

Changes proposed in this Pull Request:

Refactoring required for changes in Automattic/newspack-scripts#209. Note that this PR will not pass CI tests and should NOT be merged until Automattic/newspack-scripts#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 Automattic/newspack-scripts#209.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo added [Status] Needs Review The issue or pull request needs to be reviewed Do Not Merge! dependencies Pull requests that update a dependency file labels Jun 14, 2024
@dkoo dkoo self-assigned this Jun 14, 2024
@dkoo dkoo requested a review from a team as a code owner June 14, 2024 23:00
Copy link
Contributor

@chickenn00dle chickenn00dle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @dkoo!

I left a few comments where I ran into some issues. In addition I had one other note worth bringing up that is likely unrelated to this PR, but is still worth mentioning:

I noticed a bunch of JS errors in console while smoke testing in customizer. This happen in both trunk as well as the updated branches, and for both the current WP and 6.6

Screenshot 2024-06-20 at 13 18 26

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
chickenn00dle

This comment was marked as off-topic.

composer.lock Outdated Show resolved Hide resolved
@dkoo dkoo requested a review from miguelpeixe July 30, 2024 15:40
@miguelpeixe
Copy link
Member

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
Copy link
Contributor Author

dkoo commented Jul 30, 2024

@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.

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Aug 2, 2024
@dkoo dkoo removed the Do Not Merge! label Aug 2, 2024
@dkoo dkoo dismissed chickenn00dle’s stale review August 2, 2024 20:25

These mejs is not defined errors are no longer happening

@dkoo dkoo merged commit 1ee216c into trunk Aug 2, 2024
5 checks passed
@dkoo dkoo deleted the chore/update-dependencies branch August 2, 2024 20:32
matticbot pushed a commit that referenced this pull request Aug 2, 2024
# [2.0.0-alpha.1](v1.92.0...v2.0.0-alpha.1) (2024-08-02)

### Bug Fixes

* update dependencies to support `@wordpress/scripts` ([#2318](#2318)) ([1ee216c](1ee216c))

### BREAKING CHANGES

* Updates dependencies for compatibility with WordPress 6.6.*, but breaks JS in WordPress 6.5.* and below. If you need support for WP 6.5.*, please do not upgrade to this new major version.

* chore: refactor for newspack-scripts dependency updates

* refactor: use proxy script for eslint and stylelint scripts

* chore: update newspack-scripts to v5.6.0-alpha.3

* fix: actually update newspack-scripts to v5.6.0-alpha.3

* chore: update newspack-scripts to v5.6.0-alpha.4

* fix: semantic-release script

* fix: add missing Prettier config files

* chore: update newspack-scripts to 5.6.0-alpha.5

* chore: update newspack-scripts to v5.6.0-alpha.7

* fix: reformat SCSS

* chore: update newspack-scripts to v5.6.0-alpha.8

* fix: broken composer lockfile

* chore: bump newspack-scripts to v5.5.2
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.0.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Aug 9, 2024
# [2.0.0-epic-ras-acc.1](v1.93.0-epic-ras-acc.1...v2.0.0-epic-ras-acc.1) (2024-08-09)

### Bug Fixes

* correct linting errors ([d3b74c7](d3b74c7))
* correct more linting errors ([593c246](593c246))
* update dependencies to support `@wordpress/scripts` ([#2318](#2318)) ([1ee216c](1ee216c))

### BREAKING CHANGES

* Updates dependencies for compatibility with WordPress 6.6.*, but breaks JS in WordPress 6.5.* and below. If you need support for WP 6.5.*, please do not upgrade to this new major version.

* chore: refactor for newspack-scripts dependency updates

* refactor: use proxy script for eslint and stylelint scripts

* chore: update newspack-scripts to v5.6.0-alpha.3

* fix: actually update newspack-scripts to v5.6.0-alpha.3

* chore: update newspack-scripts to v5.6.0-alpha.4

* fix: semantic-release script

* fix: add missing Prettier config files

* chore: update newspack-scripts to 5.6.0-alpha.5

* chore: update newspack-scripts to v5.6.0-alpha.7

* fix: reformat SCSS

* chore: update newspack-scripts to v5.6.0-alpha.8

* fix: broken composer lockfile

* chore: bump newspack-scripts to v5.5.2
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Aug 13, 2024
# [2.0.0](v1.92.0...v2.0.0) (2024-08-13)

### Bug Fixes

* update dependencies to support `@wordpress/scripts` ([#2318](#2318)) ([1ee216c](1ee216c))

### BREAKING CHANGES

* Updates dependencies for compatibility with WordPress 6.6.*, but breaks JS in WordPress 6.5.* and below. If you need support for WP 6.5.*, please do not upgrade to this new major version.

* chore: refactor for newspack-scripts dependency updates

* refactor: use proxy script for eslint and stylelint scripts

* chore: update newspack-scripts to v5.6.0-alpha.3

* fix: actually update newspack-scripts to v5.6.0-alpha.3

* chore: update newspack-scripts to v5.6.0-alpha.4

* fix: semantic-release script

* fix: add missing Prettier config files

* chore: update newspack-scripts to 5.6.0-alpha.5

* chore: update newspack-scripts to v5.6.0-alpha.7

* fix: reformat SCSS

* chore: update newspack-scripts to v5.6.0-alpha.8

* fix: broken composer lockfile

* chore: bump newspack-scripts to v5.5.2
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file released on @alpha released on @epic/ras-acc released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants