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(4.x): migrate from optimist to yargs #1666

Closed
wants to merge 5 commits into from

Conversation

fabb
Copy link
Contributor

@fabb fabb commented Mar 29, 2020

Follow-up on #1662, added a few unit test on top of the original PR.

closes #1658
closes #1661

Note that there are not yet tests for all command line options. I know too little about handlebars yet (it's just a sub-sub-dependency of a package I use). If someone has good suggestions on what makes sense to test, fire away, I will update the PR.

Since I use npm audit to fail my build to not get vulnerable packages get released to production, I'm very interested into getting this merged.

Tested:

  • -f
  • --map
  • -a
  • -c
  • -h
  • -k
  • -o
  • -m
  • -n
  • -s
  • -N
  • -i
  • -r
  • -p
  • -d
  • -e
  • -b
  • -v
  • --help

@aorinevo
Copy link
Contributor

I recommend looking at yargs unit tests and maybe mimicking what is done there to test handlebars cli options.

@fabb, happy to help on this, can I get permissions to your fork?

@aorinevo
Copy link
Contributor

Seems like the unit testing structure in place will make this some what difficult.

Will try to leverage the current unit testing structure to get this done.

@fabb
Copy link
Contributor Author

fabb commented Mar 29, 2020

@aorinevo I invited you to this repo, feel free to commit to this branch, thanks for offering! I‘ll probably only get to continue on this in 2 days. I‘ll then have a look at what you‘ve done in the mean time, and at the yargs tests as you suggested. I guess for coordination it should be enough when we write here in this PR when we start and stop making changes, what do you think?

@aorinevo
Copy link
Contributor

I think that makes sense. I'll update with comments accordingly.

@aorinevo
Copy link
Contributor

aorinevo commented Mar 29, 2020

Tracking testing progress on my end via:

  • -f
  • --map
  • -a
  • -c
  • -h
  • -k
  • -o
  • -m
  • -n
  • -s
  • -N
  • -i
  • -r
  • -p
  • -d
  • -e
  • -b
  • -v
  • --help

@aorinevo
Copy link
Contributor

aorinevo commented Mar 29, 2020

@fabb, please give me a heads up before merging any changes from your end on this branch. I'd like to squash my commits into one; it's easier if there are no interwoven commits from other contributors.

@aorinevo aorinevo force-pushed the migrate-to-yargs branch 2 times, most recently from d86addc to 1e1831f Compare March 29, 2020 19:41
@aorinevo
Copy link
Contributor

@fabb, unfortunately I squashed your commits down with mine.

If you have those commits locally, I can add them back. Just push them to a branch called faab-mty-commits and I'll take it from there.

closes handlebars-lang#1658
adapted code from master to latest yargs (`.option` calls).

```
4.x:
found 188 vulnerabilities (169 low, 4 moderate, 14 high, 1 critical) in 5815 scanned packages

4.x with this PR:
found 32 vulnerabilities (17 low, 1 moderate, 13 high, 1 critical) in 5829 scanned packages
```
some indirect dependencies install @types packages which are not compatible with the older typescript. adjusted test's tsconfig to not pick these up automatically, as the actual .d.ts does not depend on these external types.
@aorinevo
Copy link
Contributor

Added back @AviVahl's commits.

@aorinevo
Copy link
Contributor

Going to pause here for a bit.

Added unit tests for 8 options. Might pick this up again later.

@fabb
Copy link
Contributor Author

fabb commented Mar 30, 2020

@fabb, unfortunately I squashed your commits down with mine.
If you have those commits locally, I can add them back. Just push them to a branch called faab-mty-commits and I'll take it from there.

I'm not here for fame, so it's fine with me ;-)

@@ -0,0 +1 @@
4.7.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can read out the dynamic value from package.json to make updating less annoying

Copy link
Collaborator

@ErisDS ErisDS Mar 30, 2020

Choose a reason for hiding this comment

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

That would really be preferable! It's possible to do e.g. require('path/to/package.json').version

Copy link
Contributor

Choose a reason for hiding this comment

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

Might require reworking the task runner but definitely doable and a better approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

another approach might be just verifying that the output is /\d+\.\d+\.\d+/ but I wouldn't worry about missing a test for this one command if it's too fiddly.

Main thing is that having this fail after every release would be a major maintenance burden 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Like most things, this ended up being pretty straight forward 🥳.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file can be removed now.

@ErisDS
Copy link
Collaborator

ErisDS commented Mar 30, 2020

@fabb and @aorinevo Thanks so much for taking this on. I'll be supporting this PR through to release.

If anyone out there regularly uses the handlebars CLI and can verify this PR is working for them, that would be fantastic.

Otherwise I'll give this a spin once you're both happy it's done.

@aorinevo
Copy link
Contributor

Thanks @ErisDS, I’ll try to get this wrapped up tonight :)

@aorinevo
Copy link
Contributor

@fabb, picking this backup now. Let me know if you have any WIP.

@aorinevo
Copy link
Contributor

aorinevo commented Mar 30, 2020

@ErisDS, I'd like to recommend skipping a unit test for -b as use of BOM is not required or recommended (see https://en.wikipedia.org/wiki/Byte_order_mark).

Thoughts?

@aorinevo
Copy link
Contributor

@ErisDS, assuming we skip -b, this PR is ready for review.

@aorinevo
Copy link
Contributor

aorinevo commented Mar 31, 2020

Cause I'm stubborn, I added the last remaining unit test for -b.

Unit test is valid as it runs against a file that includes a BOM (see image below).

Screen Shot 2020-03-30 at 8 15 53 PM

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Mar 31, 2020

Edit: In hindsight, I'm pretty sure the proper thing would have been to just wait for a review from someone who knows these things. Hiding my comment as off-topic for now. Un-hidden for context.

(Disclaimer: This is just something I tried in the volunteer spirit, while we wait for a proper review. I am not fluent with JS projects, so take this with a grain of salt/feel free to ignore if it is not helpful. Apologies in advance if that is the case.)

Should these tests pass with the old optimist-based code dropped in place of the new yargs code? I tried to do this (new tests, old code/dependencies) and it failed, but it may have been a mistake on my end.

(My Travis runs can be viewed here: https://travis-ci.com/github/DeeDeeG/handlebars.js/branches)

@aorinevo
Copy link
Contributor

@DeeDeeG, I'm rather curious myself why that's the case. I'm going to take a look.

This was referenced Mar 18, 2021
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.

6 participants