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 npm packages #645

Merged
merged 4 commits into from
Jun 8, 2021
Merged

Update npm packages #645

merged 4 commits into from
Jun 8, 2021

Conversation

htdat
Copy link
Member

@htdat htdat commented May 12, 2021

Close #620
Close #629
Close #640
Close #641

Description

Steps to Test

Looking at the E2E and JS tests for this PR https://github.com/Automattic/Edit-Flow/runs/2562511361?check_suite_focus=true

htdat added 2 commits May 12, 2021 11:06
Update lodash too. Relevant PR: #641
Command: npm install node-sass@4.14.1
Relevant: #620
Modify line numbers to apply this patch
@htdat htdat self-assigned this May 12, 2021
@htdat htdat added the Dependencies Pull requests that update a dependency file label May 12, 2021
@mikeyarce
Copy link
Member

mikeyarce commented May 31, 2021

Thanks @htdat - Is there another way to test this without relying on GitHub Actions? Just wondering if I could test this locally and what I should expect or be able to see.

@htdat
Copy link
Member Author

htdat commented Jun 1, 2021

@mikeyarce

Is there another way to test this without relying on GitHub Actions? Just wondering if I could test this locally and what I should expect or be able to see.

Yes, you can test this on your local machine but note that it's not exactly the same with GitHub Actions.

Steps to run similar tests on your local machine:

  1. First, you will need to install NodeJS on your computer. It should use v14. E.g: in my computer, it looks like this:
~ node -v
v14.15.4
  1. Make sure that you have Docker installed. Docker is used to set up test sites with the wp-env package. AFAIK, the Docker version does not matter. But here is what I have on my local machine:
docker -v
Docker version 19.03.13, build 4484c46d9d
  1. Follow all npm commands starting from this line:
    - name: Build Edit Flow

For step 3, you can replace npm ci with npm install in your local machine but in general, that does not matter much as you see the info here https://docs.npmjs.com/cli/v7/commands/npm-ci

What you can expect:

  • If you see errors when running npm ci or npm install, you can ignore them, as long as you do not see any error for npm run build.
  • If you see errors with npm run wp-env start, it's likely an error with Docker.
  • You will not see errors for Lint JS and JS tests but you may see errors for E2E tests.

Regarding E2E tests, I've found out that some tests in tests/e2e/specs/calendar-body.test.js are not stable as described here #652

Copy link
Member

@nielslange nielslange 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 different version of the dependencies, I can confirm that only npm run test-e2e shows an error when running locally. All other commands run as expected and this PR looks good to me.

package.json Outdated
"@wordpress/jest-console": "^3.6.0",
"@wordpress/jest-preset-default": "^6.0.0",
"@wordpress/jest-puppeteer-axe": "^1.7.0",
"@babel/core": "^7.14.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason, why you're not using the latest version, in this case 7.14.3, as seen on https://www.npmjs.com/package/@babel/core?

I noticed that for other dependencies there are also more recent versions available:

  • @babel/preset-env: 7.14.1 → 7.14.1
  • @testing-library/react: 10.4.9 → 11.2.7
  • @testing-library/user-event: 10.4.1 → 13.1.9

I haven't tested all dependencies, but it seems that for many of them more recent versions are available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a specific reason, why you're not using the latest version, in this case 7.14.3, as seen on https://www.npmjs.com/package/@babel/core?

I simply used npm update for all of these packages as mentioned in this commit f8c2ce1

It looks like 7.14.3 has just been released after my PR here. https://www.npmjs.com/package/@babel/core

The same should go for other dependencies.

Copy link
Member Author

@htdat htdat Jun 3, 2021

Choose a reason for hiding this comment

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

I've run npm update. All of the packages should be up-to-date now.

@htdat htdat merged commit 270d9d0 into master Jun 8, 2021
@htdat htdat deleted the update/npm-packages branch June 8, 2021 08:35
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.

None yet

3 participants