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 order of deps-related fields on package.json + add automated check #6810

Merged
merged 3 commits into from
Nov 23, 2020

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Nov 19, 2020

The first commit adds a new step to bin/check-package-metadata.js to ensure that all public packages list deps in the following order:

  1. peerDependencies
  2. dependencies
  3. devDependencies

The second commits adds a new step to bin/fix-monorepo.js to update all
public packages to list deps in the correct order.

The second commit fixes packages via bin/fix-monorepo.js to pass the new check enforced by bin/check-package-metadata.js.

This is a follow-up for the discussion in #6288.

To keep the changes small, focused and easy to review, I am intentionally NOT fixing dependencies vs. peerDependencies. I would like to open another PR to add an automated check + fix any violations discovered (e.g. in examples/greeter-extension).

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@bajtos bajtos added the Internal Tooling Issues related to our tooling and monorepo infrastructore label Nov 19, 2020
@bajtos bajtos requested a review from raymondfeng November 19, 2020 09:54
@bajtos bajtos self-assigned this Nov 19, 2020
` Expected: ${expectedStr}`,
);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Example output:

Error: Some of the packages are not following loopback-next guidelines:                                |Error: Some of the packages are not following loopback-next guidelines:
                                                                                                       |
- docs/package.json has incorrect order of keys.                                                       |- docs/package.json has incorrect order of keys.
  Actual:   devDependencies dependencies                                                               |  Actual:   devDependencies dependencies
  Expected: dependencies devDependencies                                                               |  Expected: dependencies devDependencies
- examples/greeter-extension/package.json has incorrect order of keys.                                 |- examples/greeter-extension/package.json has incorrect order of keys.
  Actual:   devDependencies dependencies                                                               |  Actual:   devDependencies dependencies
  Expected: dependencies devDependencies

@raymondfeng
Copy link
Contributor

I also would like to add a step in bin/fix-monorepo.js to fix the order of such properties in package.json.

@bajtos
Copy link
Member Author

bajtos commented Nov 20, 2020

I also would like to add a step in bin/fix-monorepo.js to fix the order of such properties in package.json.

That's a great idea, very helpful to resolve merge conflicts after some of the dependencies are updated by RenovateBot (as happened during my night) 👍

The implementation is a bit more tricky, because we need to decide where to put the block of dependency-related fields. I decided to start it after publishConfig, that option seems to be the most common case in other packages.

I reworked this PR as follows:

  1. The first commit modifying bin/check-package-metadata.js is unchanged.
  2. The second commit adds an auto-fix step to bin/fix-monorepo.js
  3. The third commit contains changes created by bin/fix-monorepo.js.

@raymondfeng LGTY now?

@bajtos bajtos requested a review from raymondfeng November 20, 2020 09:26
Add a new step to `bin/check-package-metadata.js` to ensure that all
public packages list deps in the following order:
1. `peerDependencies`
2. `dependencies
3. `devDependencies`

Signed-off-by: Miroslav Bajtoš <[email protected]>
Add a new step to `bin/fix-monorepo.js` to ensure that all
public packages list deps in the following order, starting
after `publishConfig` or `license` field:

    1. `peerDependencies`
    2. `dependencies
    3. `devDependencies`

Signed-off-by: Miroslav Bajtoš <[email protected]>
Ensure deps are specified in the following order, starting after
`publishConfig` or `license` field:

1. `peerDependencies`
2. `dependencies
3. `devDependencies`

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos merged commit ff5350c into master Nov 23, 2020
@bajtos bajtos deleted the fix/deps-order branch November 23, 2020 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants