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

Move devDependencies from individual packages to root package.json #302

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Dec 16, 2022

Details

This PR moves the devDependencies from individual Node.js packages to the root package.json file, this is because we can run the tests for both packages from the root directory instead of each package directory

Actual version bumps to the devDependencies will follow in further PRs on Monday

Additional PRs will follow for the peerDependencies changes

Ideally my initial plan is to release updated versions as v2.0.0 with as few breaking changes as possible

I originally added a Node.js job, but it fails due to a Travis CI issue, so I removed it,will add this back when switching to GitHub Actions

nvm.install
nvm install 18
node: /lib/x86_64-linux-gnu/libm.so.6: version `GLIBC_2.27' not found (required by node)
node: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.25' not found (required by node)
node: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.28' not found (required by node)
The command "npm config set spin false" failed and exited with 1 during .
Your build has been stopped.

Test Instructions

Install Node.js v16, install dependencies, run tests:

nvm install 16
nvm use 16
npm install --legacy-peer-deps
cd packages/eslint-config-humanmade
npm install --legacy-peer-deps
cd -
cd packages/stylelint-config
npm install --legacy-peer-deps
cd -
npm test

Test Results

❯ npm install --legacy-peer-deps
  cd packages/eslint-config-humanmade
  npm install --legacy-peer-deps
  cd -
  cd packages/stylelint-config
  npm install --legacy-peer-deps
  cd -
  npm test

up to date, audited 898 packages in 5s

107 packages are looking for funding
  run `npm fund` for details

7 vulnerabilities (6 moderate, 1 critical)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

up to date, audited 1 package in 56ms

found 0 vulnerabilities

up to date, audited 13 packages in 524ms

1 package is looking for funding
  run `npm fund` for details

found 0 vulnerabilities

> @humanmade/coding-standards@1.1.0 test
> npm run lint-pkg-json && npm run test:eslint && npm run test:stylelint


> @humanmade/coding-standards@1.1.0 lint-pkg-json
> npmPkgJsonLint . 'packages/*/package.json'


> @humanmade/coding-standards@1.1.0 test:eslint
> cd packages/eslint-config-humanmade && npm test


> @humanmade/eslint-config@1.2.1 test
> node fixtures/test-lint-config

Running ESLint on fixture directories. Use --verbose for a detailed report.

Linting `fixtures/fail/**`...
Warning: React version not specified in eslint-plugin-react settings. See https://github.com/yannickcr/eslint-plugin-react#configuration .
ESLint logs errors as expected.

Linting `fixtures/pass/**`...
7 files pass lint.

> @humanmade/coding-standards@1.1.0 test:stylelint
> cd packages/stylelint-config && npm test


> @humanmade/stylelint-config@1.2.1 test
> node fixtures/test-lint-config

All files that should fail log errors as expected.
No errors detected in files that are expected to pass.
~/Code/humanmade/coding-standards move-dev-dependencies                                                                                                                                                                                  8s
❯ 

@@ -4,7 +4,6 @@ import path from 'path';

import chalk from 'chalk';
import eslint from 'eslint';

Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? With it, the coding standards should actually fail on this...?

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 suspected it was a bug fix in import/order, as all 3 of these imports are external and should not have a blank line between them, particularly the package scope @ character is what was the false positive

Running the tests when including the blank line results in:

Code

import chalk from 'chalk';
import eslint from 'eslint';

import apiFetch from '@wordpress/api-fetch';

Test


/Users/netweb/Code/humanmade/coding-standards/packages/eslint-config-humanmade/fixtures/pass/import-order.js
  6:1  error  There should be no empty line within import group  import/order

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

1 unexpected error!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, so, with the current coding standards (rule config as per #219), it should require @wordpress/* imports to come separately. Which is something that has recently been discussed, and might get changed eventually (see #297).

Not sure if something changed in the ESLint rule...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll take a closer look to try to determine why the rule is failing in this PR, as based on the above it shouldn't be, thanks

Copy link
Contributor

@tfrommen tfrommen left a comment

Choose a reason for hiding this comment

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

This is looking good!

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.

None yet

2 participants