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/gutenberg jetpack and package lock #3605

Merged
merged 6 commits into from
Jun 11, 2021

Conversation

mkevins
Copy link
Contributor

@mkevins mkevins commented Jun 10, 2021

Update Jetpack and Gutenberg reference, and update package lockfile.

To test:
Tests should pass on CI.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@mkevins mkevins added the dependencies Pull requests that update a dependency file label Jun 10, 2021
@mkevins mkevins requested a review from hypest June 10, 2021 10:34
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 10, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@hypest
Copy link
Contributor

hypest commented Jun 10, 2021

👋 Matthew! Took the liberty to push the package-lock.json that my local npm install produces (node v14.17.0, npm v6.14.13 that get installed via nvm install inside the gutenberg subfolder). Let's see it that will fix the mismatch.

@hypest
Copy link
Contributor

hypest commented Jun 10, 2021

The Correctness CI job fails with some eslint issues, most probably related to outdated way we invoke it. I left a comment on a relevant PR for help but, take a look too @mkevins if you can get to it, thanks!

This aligns with the version used in the gutenberg submodule
@mkevins
Copy link
Contributor Author

mkevins commented Jun 11, 2021

Matthew! Took the liberty to push the package-lock.json that my local npm install produces (node v14.17.0, npm v6.14.13 that get installed via nvm install inside the gutenberg subfolder). Let's see it that will fix the mismatch.

Thanks Stefanos! Following up on the repllies in the thread of the relevant PR, I updated the version of eslint-pluging-jsdoc in gutenberg-mobile to align with the version used in gutenberg, and that seemingly resolved the issues we were seeing on CI.

I did not track down where / why we have our own dependency on the eslint-plugin-jsdoc, or whether it makes sense to remove it. For now, I focused on getting CI green for other PRs (especially the release PRs).

Also, for some reason when I ran npm run lint locally (which then runs eslint . --ext .js) it seemed to hang indefinitely, with CPU ~ 100%. 🤷‍♂️

@antonis antonis self-requested a review June 11, 2021 06:44
Copy link

@antonis antonis 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 @mkevins and @hypest for handling this 🙇

I updated the version of eslint-pluging-jsdoc in gutenberg-mobile to align with the version used in gutenberg, and that seemingly resolved the issues we were seeing on CI.

The change LGTM and seems to work fine since the CI is green 🎉

I did not track down where / why we have our own dependency on the eslint-plugin-jsdoc, or whether it makes sense to remove it.

That's a good point. Maybe we should revisit this 🤔

@mkevins mkevins merged commit 19725ac into develop Jun 11, 2021
@mkevins mkevins deleted the update/gutenberg-jetpack-and-package-lock branch June 11, 2021 06:57
@mkevins
Copy link
Contributor Author

mkevins commented Jun 11, 2021

Thanks for reviewing Antonis!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants