bombshell-dev / clack

Effortlessly build beautiful command-line apps
https://clack.cc
5.53k stars 90 forks source link

Fix line duplication bug with proper line wrapping #97

Closed natemoo-re closed 1 year ago

natemoo-re commented 1 year ago

Closes #87.

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: 58a1df115024788d43a19977768ebdaf43e752e2

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

This PR includes changesets to release 2 packages | Name | Type | | -------------- | ----- | | @clack/core | Patch | | @clack/prompts | 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

ulken commented 1 year ago

@natemoo-re the release build seem to have failed. Did we (I) break something by merging https://github.com/natemoo-re/clack/pull/91 or https://github.com/natemoo-re/clack/pull/95?

The latter seems more likely if so. Removing the build command seems suspicious. Is prepack not run when/as expected after all?

There were questions around your previous change there so probably related.

ulken commented 1 year ago

// cc @privatenumber

ulken commented 1 year ago

Potentially related and maybe what @natemoo-re followed?

https://github.com/changesets/changesets/issues/837#issuecomment-1222206876

ulken commented 1 year ago

Still don't understand why prepack is not called, though.

pnpm publish should trigger it if I understand this correct? https://pnpm.io/cli/publish

and changesets seem to use it if installed?

https://github.com/changesets/changesets/blob/5c53bff0efeb5bbd72976b79aefe9d5805038ebd/packages/cli/src/commands/publish/npm-utils.ts#L156

ulken commented 1 year ago

Should probably revert in the meantime?

natemoo-re commented 1 year ago

@natemoo-re the release build seem to have failed. Did we (I) break something by merging https://github.com/natemoo-re/clack/pull/91 or https://github.com/natemoo-re/clack/pull/95?

The latter seems more likely if so. Removing the build command seems suspicious. Is prepack not run when/as expected after all?

There were questions around your previous change there so probably related.

Ah whoops! I pressed merge and then stepped out. My bad! I'll have to look into what happened here.

natemoo-re commented 1 year ago

I think https://github.com/natemoo-re/clack/commit/a7e6a5a6bcc7e8aa86de29b82f5378fee4b133fe should fix it. I haven't used prepack before, maybe changesets doesn't automatically call it...?

ulken commented 1 year ago

@natemoo-re the release build seem to have failed. Did we (I) break something by merging https://github.com/natemoo-re/clack/pull/91 or https://github.com/natemoo-re/clack/pull/95?

The latter seems more likely if so. Removing the build command seems suspicious. Is prepack not run when/as expected after all?

There were questions around your previous change there so probably related.

Ah whoops! I pressed merge and then stepped out. My bad! I'll have to look into what happened here.

If anyone should apologize, it's me.

ulken commented 1 year ago

I think https://github.com/natemoo-re/clack/commit/a7e6a5a6bcc7e8aa86de29b82f5378fee4b133fe should fix it. I haven't used prepack before, maybe changesets doesn't automatically call it...?

Right, but prepack is one of many lifecycle scripts called by the package manager. pnpm supports them too.

ulken commented 1 year ago

And judging by the code changesets seems to invoke the underlying package manager.

ulken commented 1 year ago

Let's leave it at this for now. @privatenumber might want to investigate further.

Might be good to add a comment in the code?

natemoo-re commented 1 year ago

If anyone should apologize, it's me.

No worries! Hard to know if CI changes are going to work until they run... This one would have tripped me up as well.

Right, but prepack is one of many lifecycle scripts called by the package manager. pnpm supports them too.

Yeah I would expect this to work! I'm not sure why prepack wouldn't work, but better safe than sorry I guess?

natemoo-re commented 1 year ago

I'll add a comment!

ulken commented 1 year ago

TIL: https://github.com/pnpm/pnpm/issues/2891

prepack is not user-defined, though, but I wasn't aware of this difference between npm and yarn/pnpm.