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: fix modular ux and bugs #1738

Merged

Conversation

benpryke
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented May 18, 2022

🦋 Changeset detected

Latest commit: 957bbab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
modular-scripts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benpryke benpryke force-pushed the feature/fix-add-bugs-and-ux branch 3 times, most recently from a265bb2 to 14e5038 Compare May 18, 2022 16:04
@benpryke benpryke force-pushed the feature/fix-add-bugs-and-ux branch 2 times, most recently from 4d83eeb to 5662949 Compare May 18, 2022 16:42
@benpryke benpryke force-pushed the feature/fix-add-bugs-and-ux branch from 5662949 to 957bbab Compare May 18, 2022 16:44
@coveralls
Copy link
Collaborator

coveralls commented May 18, 2022

Coverage Status

Coverage decreased (-26.5%) to 0.0% when pulling 957bbab on benpryke:feature/fix-add-bugs-and-ux into 862728b on jpmorganchase:release/v3.0.2.

name: string | void;
type: string | void;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of using string | void instead of optionality (?) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I think the difference is that the field must be present in the object, but can have an undefined value, rather than simply being missing from the object entirely.

If there's a benefit there, it's marginal. I copied the pattern that was already used rather than changing it though.

name: 'name',
type: 'text',
message: `What would you like to name this package?`,
})) as { name: string } | Record<string, never>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the Record<string, never>?

Copy link
Contributor Author

@benpryke benpryke May 18, 2022

Choose a reason for hiding this comment

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

If the user cancels the prompt with Ctrl+C, we get an empty object back, literally just {}. I've used Record<string, never> to represent that empty object and allow us to check if (!response.name) on line 44, covering both the case where the user enters no name by pressing enter or cancels by pressing Ctrl+C.

Copy link
Contributor

@cristiano-belloni cristiano-belloni left a comment

Choose a reason for hiding this comment

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

lgtm

@benpryke benpryke merged commit bc95a70 into jpmorganchase:release/v3.0.2 May 19, 2022
@github-actions github-actions bot mentioned this pull request May 19, 2022
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.

3 participants