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

Add CLI Package #26

Merged
merged 34 commits into from
Jun 4, 2023
Merged

Add CLI Package #26

merged 34 commits into from
Jun 4, 2023

Conversation

ej-shafran
Copy link
Contributor

@ej-shafran ej-shafran commented May 29, 2023

Still WIP - a CLI for attw

I've been thinking of writing it in something other than TS, since it feels a little slow, but I'll probably try and add features before doing some sort of rewrite.

Currently supports:

  • Emitting raw JSON data to stdout
  • Displaying a table (similar to the one in the web version, with a few different emojis)
  • Running on a local file
  • Specifying a version

TODOS

  • Allow more table rendering options; possibly use something other than cli-table
    • Flipping the table vertically (more readable for very large tables)
    • Print results vertically (less pretty, like MySQL's -E option)
  • Handle errors!
    • Fetch errors
    • FS errors
  • Option: Exit with error code if errors are present (useful for CI)
  • Option: quiet mode
  • Read options from config file
  • Allow choosing rules to ignore/exclude

Long Term TODOs

  • Fetching from @types when no types are present
  • Get published as NPM package

@ej-shafran
Copy link
Contributor Author

I think on second thought it's probably not a good idea to add a loading state for requests; looking over command line tools that send requests I haven't found one that does that, and it seems like it would be annoying with piping and redirecting STDOUT.

The tool is a little slow, but it's at a relatively similar speed to the site, from what I've been able to check on my machine. At some point I will consider looking at possible optimizations, particularly with handling the .tar.gz file.

But I consider this basically ready? @andrewbranch, would love for you to review the code and possibly publish to NPM 🦊

Co-authored-by: Tom Ballinger <thomasballinger@gmail.com>
@andrewbranch
Copy link
Collaborator

This is super cool, thank you for working on this @ej-shafran! I will take a closer look soon, but one note is I’d like to build with tsc instead of esbuild.

@ej-shafran
Copy link
Contributor Author

@andrewbranch sure thing - I was mainly using esbuild because waiting for tsc was getting annoying :)
I'll take the time to convert it back when I have a minute

@ej-shafran
Copy link
Contributor Author

If you don't mind, though - is there any reason you'd prefer not to use esbuild?

@andrewbranch
Copy link
Collaborator

  1. Simplicity: we’re already going to invoke tsc to type check; having it emit too saves needing to configure another tool (and another direct dependency, although esbuild is already present via vite)
  2. I work on the TypeScript team and this side project is a nice way for me to dogfood our products in a real-world context 😄

@ej-shafran
Copy link
Contributor Author

@andrewbranch makes sense! I've made the change 😄

Copy link
Collaborator

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

This is looking great! I have some additional thoughts on ignore that I haven’t fully worked out, and maybe you’ll have input on. I think we also need to be able to ignore entrypoints and resolution kinds, and I’m trying to predict whether I’ll want the flexibility to ignore specific combinations of rules, entrypoints, and resolution kinds, or if blanket ignores in each of the three categories will be sufficient.

I’m thinking about introducing the concept of “profiles” in the future, which will essentially be presets for ignore. An author of an older library with lots of existing users might want a profile like max-compat that validates against everything that could be a problem for any user, but I want new libraries to be able to use a profile like esm-only to ignore the CJS entrypoint, or modern to ignore the soft-deprecated node10 resolution mode. Those examples don’t require ignoring specific rules in specific entrypoints in specific resolution kinds, so maybe an API like --ignore-rules, --ignore-entrypoints, --ignore-resolution-kinds will be flexible enough.

What do you think?

packages/cli/src/index.ts Outdated Show resolved Hide resolved
packages/cli/src/index.ts Outdated Show resolved Hide resolved
packages/cli/src/index.ts Outdated Show resolved Hide resolved
packages/cli/src/index.ts Outdated Show resolved Hide resolved
@ej-shafran
Copy link
Contributor Author

I do quite like the idea of separating the ability to ignore things by rules, entrypoints, etc. I can start by changing the current ignore to ignore-rules and adding an ignore-entrypoints.

As for the profiles - seems like a useful idea; I will try to get to those in a bit. If you have any profiles you'd like me to try and implement, let me know

@andrewbranch
Copy link
Collaborator

Hold off on the profiles for now, because I’m actually halfway through an API change in core, and will eventually backport some of the ignore stuff into core itself. In fact, it would probably make more sense to get this merged with just --ignore-rules and wait for support for the other kind of ignores to go into core.

@ej-shafran
Copy link
Contributor Author

Will do 🙃

@ej-shafran
Copy link
Contributor Author

I've fixed most of the comments you left, @andrewbranch, but I have two questions:

  1. should I move parsePackageSpec into core and just use it? see my reply to your comment
  2. what about an option that runs npm pack, checks the output, and removes the tarball file? maybe if no file-name is specified? feel like it might be nice for library creators

@andrewbranch
Copy link
Collaborator

  1. Yep! That’s fine.
  2. I think I don’t want to do that for a few reasons. (1) non-npm package managers, (2) build steps that need to be run first to produce a realistic package but won’t, (3) package.json pre- and post-pack scripts that might have unwanted side effects when the user didn’t intend to run them. Some of these concerns can be alleviated by having a prompt step that says what’s going to happen, but I don’t know if it’s worth it. On the other hand, I think it would be pretty cool to be able to do npm pack | attw to avoid leaving the tarball on disk 😎. I like taking advantage of the built-in shell environment when having a CLI.

@ej-shafran
Copy link
Contributor Author

ej-shafran commented Jun 4, 2023

Will implement 1 when I can

As for 2 - the npm pack | attw idea is cool, but I think npm pack only emits the package's name to STDOUT and never the tarball data, so piping wouldn't be possible...

It might be feasible to read .tgz data from STDIN, in case someone implements a version of npm pack that pipes to STDOUT themselves using tar piped into gzip, but might not be the most comfortable solution. That's why I think using npm pack (or at least recreating it by reading the package into an in-memory .tgz file) might be nice, though as you said that's not really useful...

@andrewbranch
Copy link
Collaborator

Oh, that makes sense. I don’t know why I was thinking the tarball stream could be piped. Oh well.

@ej-shafran
Copy link
Contributor Author

Alright then - I think this is good? Lmk if there's anything else to change, and if not give it the ol' merge and publish 🙃

@andrewbranch
Copy link
Collaborator

I added a couple commits:

  • tweaking tsconfig/build settings
  • reading the exact core and typescript versions that get used
  • made the flag for --from-npm p (for “package”) and --format -f

I think I’m also going to add some snapshot tests just to exercise all the code paths and make sure I don’t inadvertently break anything in the future.

I can’t currently push commits to the PR, I think because it’s on the main branch of your fork which probably has some default protections. I think I’ll merge this as-is and open a follow-up PR with those changes.

Thanks so much for doing this! I’m really happy with the result. 🌟

@andrewbranch andrewbranch merged commit a9c3b76 into arethetypeswrong:main Jun 4, 2023
1 check passed
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

3 participants