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: exports low level APIs #135

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

kazupon
Copy link

@kazupon kazupon commented Apr 8, 2024

πŸ”— Linked issue

rolldown/rolldown#754 (review)

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR will provide low level API runRawMain.

This makes it possible to implement command execution and error handling in the user's real parent. It also exports useful helpers and utilities for runRawMain and showUsage.

When showUsage is specified for runMain, it allows tree-shaking of citty's showUsage and renderUsage.

The function name runRawMain may be less desirable.
I tried to be making showUsage (renderUsage) customizable, but the complexity of the current implementation made generalization hard.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@pi0
Copy link
Member

pi0 commented Apr 8, 2024

Hi, and thanks for attempting this PR.

We need to properly go through exposed utils one by one in separate issues/PR and i think this one is not mergable like this. But overall:

  • formatLineColumns is really an internal primitive for example. s. It can be refactored as a consola utility for example but totally out of scope of citty to provide it.
  • We could refactor a smaller version of runMain to runMainBasic that gives hook for usage/error but I'm really worried about incosistency and usage confusion.
  • It seems we forgot the main topic in related discussion. What exact customization you expect for built-in usage about title? Maybe can you please open an issue to describe it? I can be happy to help applying it.

@pi0 pi0 marked this pull request as draft April 8, 2024 12:19
@kazupon
Copy link
Author

kazupon commented Apr 8, 2024

Thank you quick reply.

Sorry, I sent the wrong PR.
I was overworked for the issue I would like to solve.

  • It seems we forgot the main topic in related discussion. What exact customization you expect for built-in usage about title? Maybe can you please open an issue to describe it? I can be happy to help applying it.

I've put reproduction repo
https://github.com/kazupon/citty-repro1

@kazupon
Copy link
Author

kazupon commented Apr 8, 2024

sorry, I'm going to open the new issue.

@pi0
Copy link
Member

pi0 commented Apr 8, 2024

Yes i undrestand you need to override built-in showUsage and that by doing it the built-in one is still bundled.

What I'm wondering is that what can be improved for built-in showUsage to be usable by rolldown directly without needing a custom one. If you can make an issue to explain or visually share some screenshot of desired output i can help to improve built-in showUsage usable for rolldown.

(having basic run main without dependency is still good idea)

@kazupon
Copy link
Author

kazupon commented Apr 8, 2024

I am so sorry. Sound like I've have missed with my comments.
I created the issue at the following URL:
#136

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.

None yet

2 participants