Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

chore: @npmcli/package-json refactor #295

Closed
wants to merge 1 commit into from

Conversation

ruyadorno
Copy link
Contributor

Removes lib/update-root-package-json.js in favor of using
@npmcli/package-json for reading and modifying package.json during
reify.

References

Relates to: npm/statusboard#368

Removes lib/update-root-package-json.js in favor of using
@npmcli/package-json for reading and modifying package.json during
reify.

Relates to: npm/statusboard#368
@ruyadorno ruyadorno force-pushed the use-package-json-refactor branch from 7a8c339 to 15c4903 Compare June 23, 2021 16:28
@@ -31952,7 +31952,7 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin
`

exports[`test/arborist/reify.js TAP save-prod, with optional > must match snapshot 1`] = `
{"optionalDependencies":{},"dependencies":{"abbrev":"^1.1.1"}}
{"dependencies":{"abbrev":"^1.1.1"}}
Copy link
Member

Choose a reason for hiding this comment

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

:chef-kiss:

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

Consolidates "write to package json" logic and even fixes an inconsistency in the one-off implementation that was being done here before.

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

very nice! left one request, but it doesn't belong in this repo and i do not believe needs to hold up this pull request

)
})

t.test('custom formatting', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have a test for this in @npmcli/package-json, feels like we should add one. i see the code that should do this, but it's always a good idea to prove it works as intended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we actually have! 😅 I added it as part of https://github.com/npm/package-json/blob/main/test/index.js#L50 which is easy to see in the snapshot: https://github.com/npm/package-json/blob/main/tap-snapshots/test/index.js.test.cjs#L12 but I see how confusing that is from a maintenance point of view! 😬

I'll swap that test to just use regular formatting and move that check to a custom formatting separated test! thanks for bringing that up!

@isaacs isaacs closed this in 5d2624f Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants