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

@aorinevo aorinevo commented Mar 29, 2020

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

@aorinevo aorinevo commented Mar 29, 2020

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 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

@aorinevo aorinevo commented Mar 29, 2020

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

@aorinevo
Copy link
Contributor

@aorinevo 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 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 fabb:migrate-to-yargs branch 2 times, most recently from d86addc to 1e1831f Mar 29, 2020
@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 29, 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.

AviVahl added 2 commits Mar 19, 2020
closes #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 aorinevo force-pushed the fabb:migrate-to-yargs branch from d8c33f0 to e4ae377 Mar 29, 2020
@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 29, 2020

Added back @AviVahl's commits.

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 29, 2020

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 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

This comment has been minimized.

@fabb

fabb Mar 30, 2020
Author Contributor

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

This comment has been minimized.

@ErisDS

ErisDS Mar 30, 2020
Collaborator

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

This comment has been minimized.

@aorinevo

aorinevo Mar 30, 2020
Contributor

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

This comment has been minimized.

@ErisDS

ErisDS Mar 30, 2020
Collaborator

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 😅

This comment has been minimized.

@aorinevo

aorinevo Mar 30, 2020
Contributor

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

This comment has been minimized.

@aorinevo

aorinevo Mar 31, 2020
Contributor

This file can be removed now.

@ErisDS
Copy link
Collaborator

@ErisDS 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

@aorinevo aorinevo commented Mar 30, 2020

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

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 30, 2020

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

@aorinevo
Copy link
Contributor

@aorinevo 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 aorinevo force-pushed the fabb:migrate-to-yargs branch from a7ff52c to 5e6a255 Mar 30, 2020
@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 30, 2020

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

@aorinevo
Copy link
Contributor

@aorinevo 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 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

@aorinevo aorinevo commented Mar 31, 2020

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

ErisDS added a commit to ErisDS/handlebars.js that referenced this pull request Mar 31, 2020
- adds full unit tests for all cli options
- demonstrates that nothing changes between minimist and yargs except a minor order change in the --help text
- proves b9c4b25 works the same as before

Co-authored-by: fabb <fabb@users.noreply.github.com>
@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Mar 31, 2020

Manually merged via b440c38, d79212a and 80c4516

@ErisDS ErisDS closed this Mar 31, 2020
@fabb
Copy link
Contributor Author

@fabb fabb commented Mar 31, 2020

Thanks everyone!

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 31, 2020

Fancy :)

Thank you @ErisDS, @fabb, @AviVahl, and @nknapp!

@AviVahl
Copy link
Contributor

@AviVahl AviVahl commented Mar 31, 2020

Thanks all for taking this across the finishing line.

@ErisDS any chance this could be tagged and published? :)

@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Mar 31, 2020

By the way... I can confirm that, with optimist plunked in instead of yargs, all these tests (other than the --help test, as discussed above) are passing.

Here is a green/passing CI run (with optimist instead of yargs, and the --help test disabled): https://travis-ci.com/github/DeeDeeG/handlebars.js/builds/157285103

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Mar 31, 2020

I have been working on the release since I merged the changes 😉

Unfortunately, I've hit several road blocks. The latest one is that the sourcemap tests here result in the file spec/artifacts/source.map.amd.txt being overwritten. The content doesn't change but the timestamp on the file does.

The release process runs git diff-index --name-only HEAD -- which then fails and blocks the release.

I'm looking to see if I can find a fix, but I'm also wondering why that test is there twice. Is it a typo?

DeeDeeG added a commit to DeeDeeG/handlebars.js that referenced this pull request Mar 31, 2020
Reverts bin/handlebars, package-lock.json, and package.json
to the way they were before the yargs on 4.x PR.

Effectively tests that the handlebars CLI behavior is consistent
between the yargs-based and optimist-based implementations.

This reverts commit d79212a.
@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Mar 31, 2020

@ErisDS looking into that release process snag: I noticed just running the command the test runs will also write to that sourcemap file. (Minimal-ish reproducer for the issue).

handlebars -i '<div>1</div>' -a -m -N test --map './spec/artifacts/source.map.amd.txt'
(Overwrites (and updates the modification date for) ./spec/artifacts/source.map.amd.txt)

I'm not sure if there's a way to really test/run the feature without updating [the modification date of] the file.

A quick workaround would be:

git checkout HEAD -- spec/artifacts/source.map.amd.txt
(Freshly writes the file out to disk from the git index, apparently resetting git's expectation of the modification date to "now" in the process)

Kinda hacky, but it works? (To be run after the test and before git diff-index --name-only HEAD --)


Edit to add: To check only if the content of files in the project changed, and not worry about modification date, I think git diff-index is overly sensitive. git diff HEAD should do the trick for file content only.


Edit to add: I agree the duplicate test you pointed out looks like probably a typo. I deleted the duplicate test lines on my machine, and it seems to work the same.

@AviVahl
Copy link
Contributor

@AviVahl AviVahl commented Mar 31, 2020

Should probably tell the cli test to output to a temp file at os.tmpdir() and delete it afterwards (instead of trying reset a git-tracked file).

Could use fs.mkdtempSync() to get a unique folder inside temp for each run.

@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Mar 31, 2020

I don't think the test itself writes anything, other than via running handlebars.

I may be wrong, but I think handlebars is writing to a file even though there are no changes to be made. Perhaps handlebars needs to skip writing out the sourcemap file to disk if there are no changes to be made.

Edit: I suppose the sourcemap file can be copied to a tmpdir, and the test run from inside the tmpdir.

DeeDeeG added a commit to DeeDeeG/handlebars.js that referenced this pull request Mar 31, 2020
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Mar 31, 2020

In other projects, I use a folder "test-tmp/test-name" to write files generated by the tests. The advantage is that the files are easier to find and inspect if a test fails. "test-tmp" is part of gitignore and the subfolder is deleted and recreated before the test runs.

It may indeed be the case that all tests rib twice on release, but I am not sure. I think the unit tests do, for different environments.

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 31, 2020

I may be wrong, but I think handlebars is writing to a file even though there are no changes to be made. Perhaps handlebars needs to skip writing out the sourcemap file to disk if there are no changes to be made.

I can confirm that that is indeed what handlebars does.

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 31, 2020

@ErisDS, happy to take this on. I think I see a fix for this with a low-level of effort; however, I don't want to duplicate effort if you're already working on this.

Let me know :)

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 31, 2020

The fix is to use fs.unlink. That will remove the file after the test has been run.

@ErisDS, I'll prepare a PR for this.

@aorinevo
Copy link
Contributor

@aorinevo aorinevo commented Mar 31, 2020

For your reviewing pleasure :) #1667 (cc @ErisDS)

ErisDS added a commit to ErisDS/handlebars.js that referenced this pull request Apr 1, 2020
@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Apr 1, 2020

@DeeDeeG have cherry picked your commit removing the duplicate

I believe Nils' suggestion is the best approach for a fix here.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Apr 1, 2020

I've used the tmp dir approach to fix this locally and am going to push it to unblock the release.

@DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Apr 1, 2020

Thank you everyone for your efforts. This was a productive and cordial experience, and I was glad to be a part of it. Open-source can be pretty cool sometimes.

Hope everyone is safe and well out there.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented Apr 1, 2020

Hey everyone 👋 The release of 4.7.4 is now fully complete, including the various packages and updating the handlebars website.

Thank you to everyone who helped get this over the line, really appreciate it ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.