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

Support json format output with outdated command #5582

Merged
merged 17 commits into from Nov 6, 2022

Conversation

Shinyaigeek
Copy link
Member

@Shinyaigeek Shinyaigeek commented Nov 3, 2022

Summary

Support json format output with outdated command

Ticket

Closes: #2705

Details

support JSON format output with outdated command if --format json is specified.

Test

I tested this with my own project,

pnpm outdated --format json
{
        "@types/yargs": {
                "current": "17.0.11",
                "latest": "17,0.13",
                "dependencyKind": "dev"
        },
        "dotenv": {
                "current": "16.0.1",
                "latest": "16,0.3",
                "dependencyKind": "dev"
        },
        "jsdom": {
                "current": "20.0.0",
                "latest": "20,0.2",
                "dependencyKind": "dev"
        },
        "tslib": {
                "current": "2.4.0",
                "latest": "2,4.1"
        }
}

Comment on lines 41 to 42
table: Boolean,
format: ['json'],
Copy link
Member Author

Choose a reason for hiding this comment

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

If we support this feature, it may be better to merge table and format options. such as format: ['json', 'table', 'list']

Copy link
Member Author

Choose a reason for hiding this comment

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

adding this, I also think this would be good to support markdown table format with outdated command. I would like to use outdated command's output with GitHub's issue, and it will be helpful to support markdown table format

Copy link
Member

Choose a reason for hiding this comment

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

If we support this feature, it may be better to merge table and format options. such as format: ['json', 'table', 'list']

sounds good. Regarding markdown, I am not sure how frequently that would be needed.

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 added the process to handle table and list format option in 5d82338. Should we deprecate table option in outdated command?

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the markdown table format, the only use case I can come up with is copying and pasting the output into a GitHub issue. I feel it is good to support this, but should we support this? (Of course this will be implemented in the other PR if we will do)

@zkochan
Copy link
Member

zkochan commented Nov 3, 2022

Sounds good

@Shinyaigeek Shinyaigeek force-pushed the feature/support-format-option-for-outdated-command branch from 3951b2b to 4325363 Compare November 4, 2022 17:30
@Shinyaigeek Shinyaigeek changed the title [PoC] Support json format output with outdated command Support json format output with outdated command Nov 4, 2022
@Shinyaigeek Shinyaigeek force-pushed the feature/support-format-option-for-outdated-command branch from 4325363 to 70a69bb Compare November 4, 2022 17:46
@Shinyaigeek Shinyaigeek self-assigned this Nov 4, 2022
Comment on lines 253 to 254
function renderOutdatedJSON (outdatedPackages: readonly OutdatedPackage[], opts: { long?: boolean }) {
const outdatedPackagesJSON = {} as {
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 wondered whether I should create a function that is almost equivalent to renderLatest, only without the output with chalk, but I was concerned that the implementation would be scattered, so I implemented it by adding withoutChalk argument which disables chalk output

@Shinyaigeek Shinyaigeek marked this pull request as ready for review November 4, 2022 17:50
@Shinyaigeek
Copy link
Member Author

@zkochan first implementation is done!, so please review this when you are free 🙇

@@ -289,26 +326,32 @@ export function renderCurrent ({ current, wanted }: OutdatedPackage) {
return `${output} (wanted ${wanted})`
}

export function renderLatest (outdatedPkg: OutdatedWithVersionDiff): string {
export function renderLatest (outdatedPkg: OutdatedWithVersionDiff, withoutChalk = false): string {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any benefit from using this function. Just create a new one.

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 was concerned about a scattering of implementation, but this is not a firm intention, so fixed in 90069a4 👍

if (latestManifest == null) return ''
const outputs = []
if (latestManifest.deprecated) {
outputs.push(wrapAnsi(chalk.redBright(latestManifest.deprecated), 40))
const detail = latestManifest.deprecated
outputs.push(wrapAnsi(withoutChalk ? detail : chalk.redBright(detail), 40))
Copy link
Member

Choose a reason for hiding this comment

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

why do you need to wrap for the json output?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, just mistake. 90069a4 👍

return colorizeSemverDiff({ change, diff })
}

export function renderDetails ({ latestManifest }: OutdatedPackage) {
export function renderDetails ({ latestManifest }: OutdatedPackage, withoutChalk = false) {
Copy link
Member

Choose a reason for hiding this comment

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

create a new function for json

Copy link
Member Author

Choose a reason for hiding this comment

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

same here #5582 (comment)

[outdatedPackageName: string]: {
current: string
latest: string
dependencyKind?: 'dev' | 'optional'
Copy link
Member

Choose a reason for hiding this comment

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

why is belongsTo not good?

if you want to keep it, rename it to dependencyType and probably make it required.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/pnpm/pnpm/blob/main/packages/plugin-commands-outdated/src/outdated.ts#L278 I was going to fit to renderPackageName, but this is not a firm intention. fixed in 5abaceb

@@ -153,6 +157,39 @@ ${renderCurrent(outdatedPkg)} ${chalk.grey('=>')} ${renderLatest(outdatedPkg)}`
.join('\n\n') + '\n'
}

function renderOutdatedJSON (outdatedMap: Record<string, OutdatedInWorkspace>, opts: { long?: boolean }) {
Copy link
Member

Choose a reason for hiding this comment

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

why isn't the similar function from outdated.ts reused here?

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 was wondering which or not I should re-implement renderOutdatedJSON also in recursive.ts or not. I decided to re-implement it because

  • in recursive, dependentPackages field is added and this requires OutdatedInWorkspace (but this is trivial)
  • sortOutdatedPackages function, which is used in both of outdated and recursive, is a bit different in outdated and recursive…

\t\t"latest": "3.1.0",
\t\t"dependencyKind": "dev"
\t}
}`)
Copy link
Member

Choose a reason for hiding this comment

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

it looks pretty ugly. You could just run JSON.stringify to generate the expected output.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea! Thank you! 2f3f4f5 👍

Comment on lines 226 to 248
const { output, exitCode } = await outdated.handler({
...OUTDATED_OPTIONS,
dir: process.cwd(),
long: true,
table: false,
})

expect(exitCode).toBe(1)
expect(stripAnsi(output)).toBe(`@pnpm.e2e/deprecated
1.0.0 => Deprecated
This package is deprecated. Lorem ipsum
dolor sit amet, consectetur adipiscing
elit.
https://foo.bar/qar

is-negative
1.0.0 => 2.1.0
https://github.com/kevva/is-negative#readme

is-positive (dev)
1.0.0 => 3.1.0
https://github.com/kevva/is-positive#readme
`)
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? This format already existed before your changes. Why do you add a test for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, just mistake. c92fd62 👍

Comment on lines 41 to 42
table: Boolean,
format: ['json'],
Copy link
Member

Choose a reason for hiding this comment

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

If we support this feature, it may be better to merge table and format options. such as format: ['json', 'table', 'list']

sounds good. Regarding markdown, I am not sure how frequently that would be needed.

for (const outdatedPkg of sortOutdatedPackages(outdatedPackages)) {
const { packageName, belongsTo } = outdatedPkg
outdatedPackagesJSON[packageName] = {
current: renderCurrent(outdatedPkg),
Copy link
Member

Choose a reason for hiding this comment

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

It is a JSON, so I would rather return the unprocessed fields as they are. Like:

Suggested change
current: renderCurrent(outdatedPkg),
current: outdatedPkg.current,
wanted: outdatedPkg.wanted,

Copy link
Member Author

Choose a reason for hiding this comment

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

Certainly!! This is about JSON format and this is usually used with automation process, so it is better to simplify. 2299772 👍

const { packageName, belongsTo } = outdatedPkg
outdatedPackagesJSON[packageName] = {
current: renderCurrent(outdatedPkg),
latest: extractLatest(outdatedPkg),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a function. Just return

Suggested change
latest: extractLatest(outdatedPkg),
latestVersion: outdatedPkg.latestManifest?.version,
latestDeprecated: outdatedPkg.latestManifest?.deprecated,

Copy link
Member Author

Choose a reason for hiding this comment

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

same as #5582 (comment)

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 thought user want to know which package is deprecated, but they only know which package is deprecated by checking current field is equal to latestVersion field and this is not friendly. So I added deprecated field.

}

if (opts.long) {
outdatedPackagesJSON[packageName].details = extractDetails(outdatedPkg)
Copy link
Member

Choose a reason for hiding this comment

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

I think for long we can just pass the whole latest manifest

Suggested change
outdatedPackagesJSON[packageName].details = extractDetails(outdatedPkg)
outdatedPackagesJSON[packageName].latestManifest = outdatedPkg.latestManifest

Copy link
Member Author

Choose a reason for hiding this comment

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

same as #5582 (comment)

@zkochan zkochan force-pushed the feature/support-format-option-for-outdated-command branch from e8bcde1 to 72b4d27 Compare November 6, 2022 01:14
@zkochan zkochan merged commit 46852d4 into main Nov 6, 2022
@zkochan zkochan deleted the feature/support-format-option-for-outdated-command branch November 6, 2022 01:37
@zkochan zkochan added this to the v7.15 milestone Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support --json for pnpm outdated
2 participants