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

feat(cli): on update process add flag to opt-out from publishing #415

Merged
merged 7 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 53 additions & 36 deletions packages/cli/src/__tests__/main.test.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,53 @@
import { createCLI } from '../main'
import { create, add, deploy } from '../commands'
import { vi } from 'vitest'
import { describe, vi } from 'vitest'

vi.mock('../commands')

const DEFAULT_ARGS = ['node', 'main.ts'] // cli.parse() should receive the executable and its arget as its first two arguments.

describe('main', () => {
describe('default arguments', () => {
it('create command', () => {
const cli = createCLI()
cli.parse([...DEFAULT_ARGS, 'create'])
expect(create).toHaveBeenCalledWith({ dir: '.' })
})

it('add command', () => {
describe('add command', () => {
it('default arguments', () => {
const cli = createCLI()
cli.parse([...DEFAULT_ARGS, 'add'])
expect(add).toHaveBeenCalledWith({ dir: '.' })
})

it('deploy command', () => {
it('full arguments', () => {
const cli = createCLI()
cli.parse([...DEFAULT_ARGS, 'deploy'])
expect(deploy).toHaveBeenCalledWith({ dir: '.', skipPrompts: false })
cli.parse([
...DEFAULT_ARGS,
'add',
'--template',
'react',
'--name',
'my-test-plugin',
'--dir',
'my-directory',
'--structure',
'standalone',
'--packageManager',
'npm',
])
expect(add).toHaveBeenCalledWith({
template: 'react',
name: 'my-test-plugin',
dir: 'my-directory',
structure: 'standalone',
packageManager: 'npm',
})
})
})

describe('full arguments', () => {
it('create command', () => {
describe('create command', () => {
it('default arguments', () => {
const cli = createCLI()
cli.parse([...DEFAULT_ARGS, 'create'])
expect(create).toHaveBeenCalledWith({ dir: '.' })
})

it('full arguments', () => {
const cli = createCLI()
cli.parse([
...DEFAULT_ARGS,
Expand All @@ -49,33 +68,30 @@ describe('main', () => {
repoName: 'my-test-repo',
})
})
})

it('add command', () => {
describe('deploy command', () => {
it('--no-publish', () => {
const cli = createCLI()
cli.parse([
...DEFAULT_ARGS,
'add',
'--template',
'react',
'--name',
'my-test-plugin',
'--dir',
'my-directory',
'--structure',
'standalone',
'--packageManager',
'npm',
])
expect(add).toHaveBeenCalledWith({
template: 'react',
name: 'my-test-plugin',
dir: 'my-directory',
structure: 'standalone',
packageManager: 'npm',
cli.parse([...DEFAULT_ARGS, 'deploy', '--no-publish'])
expect(deploy).toHaveBeenCalledWith({
dir: '.',
publish: false,
skipPrompts: false,
})
})

it('default options', () => {
const cli = createCLI()
cli.parse([...DEFAULT_ARGS, 'deploy'])
expect(deploy).toHaveBeenCalledWith({
dir: '.',
skipPrompts: false,
publish: true,
})
})

it('deploy command', () => {
it('full arguments', () => {
const cli = createCLI()
cli.parse([
...DEFAULT_ARGS,
Expand All @@ -100,6 +116,7 @@ describe('main', () => {
name: 'my-test-plugin',
output: './dist/index.js',
dir: 'my-directory',
publish: true,
dotEnvPath: '.',
scope: 'my-plugins',
})
Expand Down
44 changes: 39 additions & 5 deletions packages/cli/src/commands/deploy/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ const packageNameMessage =
'How would you like to call the deployed field-plugin?\n (Lowercase alphanumeric and dash are allowed.)'

type UpsertFieldPluginFunc = (args: {
scope: Scope
token: string
dir: string
packageName: string
skipPrompts?: boolean
token: string
output: string
scope: Scope
skipPrompts: undefined | boolean
publish: boolean
}) => Promise<{ id: number }>

type GetPackageName = (params: {
Expand All @@ -42,7 +43,7 @@ type GetPackageName = (params: {

// TODO: move all side effects to the deploy function
export const upsertFieldPlugin: UpsertFieldPluginFunc = async (args) => {
const { packageName, skipPrompts, token, output, dir, scope } = args
const { packageName, skipPrompts, token, output, dir, scope, publish } = args

const manifest = loadManifest()

Expand Down Expand Up @@ -78,9 +79,11 @@ export const upsertFieldPlugin: UpsertFieldPluginFunc = async (args) => {
await confirmOptionsUpdate(manifest?.options)
}

const shouldPublish = await checkPublish({ publish, skipPrompts })

await storyblokClient.updateFieldType({
id: fieldPluginFound.id,
publish: true,
publish: shouldPublish,
field_type: {
body: output,
options: manifest?.options,
Expand Down Expand Up @@ -378,6 +381,7 @@ export const createFieldPlugin = async (
//we need to force an update call right after the creation.
//If no options is found, it's not going to be sent to the API since undefined
//properties are not encoded.
//NOTE: The `publish` property is set to true here because it is a part of the creation process and should provide a consistent flow.
await client.updateFieldType({
id: fieldPlugin.id,
publish: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100% happy about this as it is a piece of code that might easily get overlooked, but I think keeping it to publish:true makes sense here, as it kind of mimics the creation process. So I would suggest keeping it to not confuse our users.

Expand Down Expand Up @@ -414,3 +418,33 @@ export const createFieldPlugin = async (
)
}
}

// Publish Logic

type CheckPublish = (params: {
publish: boolean
skipPrompts: undefined | boolean
}) => Promise<boolean>

export const checkPublish: CheckPublish = async ({ publish, skipPrompts }) => {
if (skipPrompts === true || publish === false) {
return publish
demetriusfeijoo marked this conversation as resolved.
Show resolved Hide resolved
}

//NOTE: In interactive mode where --no-publish is not provided, the user is asked to confirm the publish action.
const publishConfirmation = await confirmPublish()
return publishConfirmation
}

const confirmPublish = async (): Promise<boolean> => {
const { publish } = await betterPrompts<{
publish: boolean
}>({
type: 'confirm',
name: 'publish',
message: 'Do you want to publish a new version of the field plugin?',
initial: false,
})

return publish
}
5 changes: 4 additions & 1 deletion packages/cli/src/commands/deploy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { Scope } from '../../storyblok/storyblok-client'
export type DeployArgs = {
skipPrompts: boolean
dir: string
publish: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

@BibiSebi is this value coming from the flag --no-publish? Maybe I missed it, but where the flag is --no-publish is giving the value to this boolean? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah its rather weird and unintuitive but when defining -noo-publish option with commander.js

it will automatically translate it into publish:false and if you do not provide the option then it will be per default publish:true

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, next level of a smart tool 🤯 Love it!

name: undefined | string
token: undefined | string
output: undefined | string
Expand All @@ -25,7 +26,8 @@ export type DeployArgs = {
type DeployFunc = (args: DeployArgs) => Promise<void>

export const deploy: DeployFunc = async (params) => {
const { skipPrompts, token, name, dir, output, dotEnvPath, scope } = params
const { skipPrompts, token, name, dir, output, publish, dotEnvPath, scope } =
params

console.log(bold(cyan('\nWelcome!')))
console.log("Let's deploy a field-plugin.\n")
Expand Down Expand Up @@ -106,6 +108,7 @@ export const deploy: DeployFunc = async (params) => {
token: personalAccessTokenResult.token,
output: outputFile,
scope: apiScope,
publish,
})

console.log(
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export const createCLI = () => {
'--name <value>',
'name of plugin (Lowercase alphanumeric and dash)',
)
.option(
Copy link
Contributor Author

@BibiSebi BibiSebi Oct 2, 2024

Choose a reason for hiding this comment

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

I tried out this approach as described in commander.js --> https://www.npmjs.com/package/commander#other-option-types-negatable-boolean-and-booleanvalue

However I might rewrite it depending on your feedback.
The better solution to this I see is to have --publish=true|false however then --publish without value might not work. And I will need to parse and validate boolean values, but thats fine, just a heads-up :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach looks good to me, Bibi. 🤔

I wonder though, what if we have just the --no-publish option, and in case it's not passed (meaning it will be true), we always prompt the user to confirm the publishing unless the --skipPrompts is also present?

The combinations would be something like:

yarn deploy (which in case of an update, always asks if the user wants or not to publish)
yarn deploy --skipPrompts (publish without asking anything)
yarn deploy --no-publish (does not publish and does not show the prompt)
yarn deploy --no-publish --skipPrompts (does not publish and not show any prompt)

I didn't think deeply about it, so I may be missing some pieces 🤣 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might also work. Let's see what @eunjae-lee and @Dawntraoz think as well :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a slight breaking change. Let's imagine someone was using deploy command with all the options except for --skipPrompts, but since all the options were given, they didn't encounter any prompt, and they used it for their CI. Now we're changing the default behavior to prompt about publishing. Then their CI will break.

So I think we can keep publishing by default, and just provide --no-publish, then it would also be clear that publishing is done by default. What do you think? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so you would also suggest removing --publish? Works for me :) I will adjust it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demetriusfeijoo @eunjae-lee here are the updates
a5a690e

'--no-publish',
'This flag is only applied when a plugin is being updated! Uploads field plugin but does not publish a new version.',
)
.addOption(
new Option(
'--output <value>',
Expand Down
Loading