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

Update Newspack blocks to v1.37.1 #56121

Merged
merged 2 commits into from
Sep 9, 2021
Merged

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Sep 8, 2021

Changes proposed in this Pull Request

Bump newspack blocks to v1.37.1 and resolve peer dependency warnings via .yarnrc.yml.

Releasing to npm

Here's how I did the release to npm:

  1. Cloned the newspack blocks repo
  2. Checked out the commit tagged with the 1.37 release
  3. Changed the "name" field in package.json to include @automattic/ as a prefix
  4. Removed the private: true field in package.json.
  5. Ran the following commands:
npm ci
npm run clean
NODE_ENV=production npm run build

npm publish

Testing instructions

  1. Sync the build to your sandbox (install-plugin.sh etk update/newspack-blocks-1.37)
  2. Load the editor in a sandboxed site, and verify the ETK version in the "more menu" dropdown matches the version on your sandbox.
  3. Smoke test inserting and using the newspack blocks.

@noahtallen noahtallen self-assigned this Sep 8, 2021
@noahtallen noahtallen requested review from dkoo and a team September 8, 2021 22:35
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 8, 2021
@github-actions
Copy link

github-actions bot commented Sep 8, 2021

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit update/newspack-blocks-1.37 on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@matticbot
Copy link
Contributor

matticbot commented Sep 8, 2021

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~17 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding        -77 B  (-0.0%)      -17 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Async-loaded Components (~15 bytes removed 📉 [gzipped])

name                                            parsed_size           gzip_size
async-load-design-wordpress-components-gallery        -77 B  (-0.0%)      -15 B  (-0.0%)
async-load-block-editor                               -77 B  (-0.0%)      -15 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

react: '*'
'@wordpress/[email protected]':
peerDependencies:
react-native: '*'
'@wordpress/[email protected]':
peerDependencies:
react-native: '*'
'@wordpress/[email protected]':
peerDependencies:
react-native: '*'
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, do you know why api-fetch peer-depends on react-native? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is one wp package which has react-native as a peer dependency, and that causes everything else which uses that package to show a warning. Since a lot of wp packages depend on that package but do not include react-native, yarn says that the peer dependency requirements are not met. Adding it to the peer dependencies in this file basically indicates to yarn that we (the calypso repo) will provide that peer dependency, thus resolving the peer dep requirement.

That said, I think this issue doesn't exist with newer versions of WP packages, so it's possible we can remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just double checked. It hasn't been published yet, but WordPress/gutenberg#34687 will resolve this issue. The exact problem is:

  • react-native-url-polyfill is added by @wordpress/url
  • react-native-url-polyfill has a peer dep on react-native, which isn't provided by @wordpress/url
  • Everything else including @wordpress/url (such as @wordpress/api-fetch) now has an invalid peer dependency requirement.
  • These are fulfilled by adding react-native as a peer dep to each package depending on @wordpress/url, and then providing react-native at the top level ourselves

Copy link
Contributor

@fullofcaffeine fullofcaffeine left a comment

Choose a reason for hiding this comment

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

Tested well from the sandbox. Some unrelated tests are failing and I re-triggered them just in case.

@noahtallen noahtallen changed the title Update Newspack blocks to v1.37 Update Newspack blocks to v1.37.1 Sep 9, 2021
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Smoke test looks good! 👍

@noahtallen noahtallen merged commit 1a0a314 into trunk Sep 9, 2021
@noahtallen noahtallen deleted the update/newspack-blocks-1.37 branch September 9, 2021 22:46
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants