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 support @wordpress/scripts #209

Merged
merged 32 commits into from
Aug 2, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jun 13, 2024

All Submissions:

Changes proposed in this Pull Request:

Some long-needed housecleaning. This PR supports changes across all Newspack repositories which rely on newspack-scripts for build, linting, and release processes. Changes encompassed:

  • Updates as many dependencies as possible
  • Swaps out Calypso Build in favor of the newer and more maintained @wordpress/scripts due to peer dependency conflicts between WP 6.6 and calypso-build
  • Changes NPM support from v16 to lts/* (the latest Long Time Support version—currently v20)
  • Moves as many shared dependencies as possible from Newspack repos to this repository
  • Updates config and script files as needed to accommodate updated dependency versions

These updates should not result in any functional changes. Relates to the following PRs in other repos:

How to test the changes in this Pull Request:

  1. Make sure your testing site is running WP 6.6
  2. Clone all chore/update-dependency branches from the repos listed above to your local dev machine.
  3. Run nvm use to switch your Node to the latest compatible version (you are using NVM, right?).
  4. Run rm -rf node_modules && npm ci --legacy-peer-deps && composer install to reinstall PHP and JS dependencies.
  5. For each repository, test workflow scripts:
  • npm run watch to start webpack in devserver/watch mode
  • npm run build to build production JS/CSS files
  • npm run lint to lint JS and SCSS files (hint: if these aren't returning any results, try temporarily introducing some code changes which will throw errors, such as an early return; in JS or single quotes in SCSS)
  • npm run fix:js to correct autofixable ESLint errors (may need to introduce temporary changes to test this)
  • npm run format:js to autoformat JS according to Prettier rules: Note that this will probably result in lots of changes, but we don't need to commit those changes at this time.
  • npm run format:scss to autoformat SCSS according to Stylelint rules (may need to introduce temporary changes to test this)
  • npm run release to test prerelease processes
  • npm run test to run JS unit tests (Plugin, Blocks, Newsletters, Popups only)
  • npm run typescript:check to run tsc (Plugin, Blocks only)
  1. For each repository, smoke test major functionality in both admin and front-end contexts to make sure the built JS and CSS still work as expected. e.g. poke around the Newspack dashboard, create some newsletters and Campaigns prompts, update theme Customizer options, browse the site front-end as a reader, etc.

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 dependencies Pull requests that update a dependency file [Status] Needs Review labels Jun 14, 2024
@dkoo dkoo self-assigned this Jun 14, 2024
@dkoo dkoo marked this pull request as ready for review June 14, 2024 23:08
@dkoo dkoo requested a review from a team as a code owner June 14, 2024 23:08
@leogermani
Copy link
Contributor

leogermani commented Jul 2, 2024

I ran tests in all repos, first trying to run all the scripts, and then doing some functional tests running the plugins. Here's what I found:

ps - I don't know which of these thigns are expected and I didn't investigate how to fix anything yet.

Scripts

  1. Newspack Lists fails to build:
[webpack-cli] HookWebpackError: error:0308010C:digital envelope routines::unsupported
    at makeWebpackError (/newspack-repos/newspack-listings/node_modules/webpack/lib/HookWebpackError.js:48:9)
    at /newspack-repos/newspack-listings/node_modules/webpack/lib/Compilation.js:3057:12
    at eval (eval at create (/newspack-repos/newspack-listings/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:68:1)
  1. super-cool-ad-inserter-plugin and republication-tracker-tool don't run any scripts. They just say Missing scripts

Functional tests

  1. I see this Warning in several (but not all) places
Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.
  1. newspack-blocks blocks are not being registered. And I see this error:

Blocks are working, but the console is full of warnings like this

Warning: Encountered two children with the same key, `undefined-selector`.
  1. I also see this warning in some of the wizards:
Store "newspack/wizards" is already registered.

This is what I've found so far

@leogermani
Copy link
Contributor

I've updated the comment above. Blocks are working

@dkoo dkoo changed the title fix: update dependencies to avoid conflict in WP 6.6 fix: update dependencies to support @wordpress/scripts Jul 29, 2024
@dkoo
Copy link
Contributor Author

dkoo commented Jul 29, 2024

All repos updated with the latest trunk once again. To respond to @leogermani's comment above,

Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.

I'm seeing this everywhere too. I think it's due to updating the React dependency to v18. It's kind of annoying because it clogs up the console, but doesn't seem to cause any actual issues. I think we should address this with a refactor to remove defaultProps wherever we use them after this PR lands.

Store "newspack/wizards" is already registered.

This was already happening to me on trunk so I don't think it's related to the changes here. I also see Jetpack throwing a similar JS console error. Again, it doesn't seem to cause any functional issues, but maybe it's something we can fix after this PR lands.

Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Other than the issues with the js formatter and ts checker, everything seems to be working well across all plugins.

I wasn't able to fully test the npm run release command due to environment restrictions:

[16:29:07] [semantic-release] › ⚠  This run was not triggered in a known CI environment, running in dry-run mode.
[16:31:01] [semantic-release] › ℹ  This test run was triggered on the branch chore/update-dependencies, while semantic-release is configured to only publish from release, alpha, hotfix/image-credits-hpp, hotfix/subs-limiting, hotfix/update-js-version-strategy, hotfix/update-subtitle-selector, epic/consolidate-data-flows, epic/ras-acc, epic/ras-acc-test-dependency-updates, therefore a new version won’t be published.
[newspack-scripts@5.6.0-alpha.8] No release published.

It does reach the release script, which only got formatting changes. It's safe to assume it'll work fine but we should keep an eye out when running it live.

Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

It's important to note that these changes break all our wizards and blocks on WP <= 6.5.5, as also reported by @chickenn00dle. Not sure how we could prevent the issue, since it looks like an issue with dependencies changes from WP itself.

image image

@dkoo
Copy link
Contributor Author

dkoo commented Aug 1, 2024

@miguelpeixe Thanks for the additional testing and review!

Other than the issues with the js formatter and ts checker, everything seems to be working well across all plugins.

I think we'll probably not want to use the format:js command for a while, because the Prettier config in @wordpress/scripts enforces very different code formatting than what we've been accustomed to as a team (e.g. no spaces in brackets, different whitespace and indentation rules, etc.). That's why running npm run format:js results in a massive amount of formatting changes to almost all JS files.

Still, I kept the script around so that we could have the option to switch our code style to more closely match @wordpress/scripts formatting in the future. For now, I've actually disabled Prettier rules in our ESLint config so that formatting is essentially ignored for linting purposes.

I wasn't able to fully test the npm run release command due to environment restrictions

Yeah, I think there's no way around this for now. Once we release this PR as a production NPM package and merge the other PRs, we can test by creating new alphas for every repo, which will be needed anyway in order to release the updates in all repos to production.

It's important to note that these changes break all our wizards and blocks on WP <= 6.5.5, as also Automattic/newspack-blocks#1774 (review). Not sure how we could prevent the issue, since it looks like an issue with dependencies changes from WP itself.

Yes, this was actually the initial motivation behind these dependency updates. Initially, before @adekbadek found the regenerator-runtime workaround, our plugins were all completely broken in WP 6.6+ due peer dependency conflicts between calypso-build and the newer Babel and React packages in WP 6.6+. After the regenerator-runtime fix our plugins do work in both WP 6.5.* and 6.6.*, but we're not certain how long that will remain true, and I think we should update for parity with the latest WP core version sooner rather than later. WP 6.7 is scheduled for early November, and who knows what other dependency updates that version will bring?

Because updating to @wordpress/scripts does break our code in WP versions below 6.6, we'll need to release this as new major versions highlighting that they contain BREAKING CHANGES for older WP versions. Not so much a concern for our own hosted sites, as everyone is now on WP 6.6+, but still important for the OSS community.

Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Thank you for this massive work and thorough explanations!

I've tested a few more times across all plugins and it's running smoothly 💯

@dkoo dkoo merged commit eea9df9 into trunk Aug 2, 2024
4 checks passed
@dkoo dkoo deleted the epic/update-dependencies branch August 2, 2024 18:25
@matticbot
Copy link

🎉 This PR is included in version 5.5.2 🎉

The release is available on:

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 [Status] Approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants