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

Feature/typescript #130

Merged
merged 1 commit into from
Dec 1, 2020
Merged

Feature/typescript #130

merged 1 commit into from
Dec 1, 2020

Conversation

brandongregoryscott
Copy link
Contributor

@brandongregoryscott brandongregoryscott commented Nov 4, 2020

Closes #14 Investigate feasibility/benefit of porting project to Typescript

Initial pass of the TS port for the project. All automated tests should be updated & passing. There are likely some missed areas of type narrowing/specification that can be filled in later.

A few things of note:

  • I opted to not use ts-node for development. I was unable to use just ts-node to run anything but the main entrypoint file, and subcommands were not registered. It required a much longer command string pointing to the tsconfig file (see ts-node is not running sub-command tj/commander.js#955 (comment) for an example)
    • With that in mind, while developing, it'll be the smoothest experience to run the watch npm script as the and-cli-dev alias now points to <repo>/dist/and-cli.js
    • Integration test files were converted over to .ts, but the TestUtils function that executes the CLI commands will also reference the dist folder for executable js files.
  • Jest was slightly upgraded (to the latest released version of v25), but ts-jest is throwing warnings about using jest <26. I believe jest v26+ requires node 10 and above, so we'd need to agree to move forward with that upgrade as a team - there are some implications with our Jenkins server that is still on node 8.16.3
  • All modules were capitalized to follow our standard convention with utility classes (CoreUtils, CollectionUtils, etc.). So any plugin projects or older branches referencing them will need to be updated.
  • Additionally, the main entrypoint (and-cli.ts) will now export all of the shared modules for consumer use, similar to how our andculturecode-javascript-core and andculturecode-javascript-react packages export everything in the index.ts file.

@wintondeshong This is a massive PR, so I don't expect you to scan everything line-by-line (some of the restructuring caused git to register the changes as new files entirely), but I'd love to get your input on some of this, too. In terms of releasing it, I was thinking we could release it as 2.0.0-beta version(s) until we iron out any of the potential regressions with the port - what do you think?

  • Related GitHub issue(s) linked in PR description
  • Destination branch merged, built and tested with your changes
  • Code formatted and follows best practices and patterns
  • Code builds cleanly (no additional warnings or errors)
    • See above notes about the ts-jest warnings
  • Manually tested
  • Automated tests are passing
  • [-] No decreases in automated test coverage
    • Codecov is reading a small decrease in %, but there shouldn't be a ton of new code. We can revisit the coverage after the initial port is in.
  • Documentation updated (readme, docs, comments, etc.)
  • Localization: No hard-coded error messages in code files (minimally in string constants)
Instructions for beta-testing
# Ensure latest and-cli is installed for access to workspace command
npm i -g and-cli@latest

# Ensure you are in a directory appropriate for holding OSS repos (can be anywhere, just an example)
cd ~/andculturecode/forks/

# Clone all of my forks
and-cli workspace -u brandongregoryscott && cd brandongregoryscott.AndcultureCode.Cli

# Fetch remote branches and checkout the typescript branch
git fetch && git checkout feature/typescript

# Install dependencies and run a build
npm install && npm run build

# Install the CLI aliases
./dist/and-cli.js install

Now you can run the built JS version of the CLI by running `and-cli-dev`
Command audit/testing checklist
  • copy
    • -d, --destination <destination>
    • -f, --flags <options>
    • -s, --source <source>
  • deploy
    • aws-beanstalk
      • --dotnet
      • --timeout <timeout>
      • --verbose
    • aws-s3
      • --destination <destination>
      • --profile <profile>
      • --public-url <url>
      • --publish
      • --source <source>
      • --webpack
    • azure-storage
      • --client-id <clientID>
      • --destination <destination>
      • --public-url <url>
      • --publish
      • --recursive
      • --secret <profile>
      • --source <source>
      • --tenant-id <tenantID>
      • --username <username>
      • --webpack
    • azure-web-app
      • --app-name <applicationName>
      • --branch <branch>
      • --client-id <clientID>
      • --force
      • --remote <remote>
      • --resource-group <resourceGroup>
      • --secret <profile>
      • --tenant-id <tenantID>
      • --username <username>
    • jenkins
      • --new
      • -i, --init
      • -p, --profile <profile>
  • dotnet
    • -b, --build
    • -c, --clean
    • -C, --cli
    • -k, --kill
    • -p, --publish
    • -R, --restore
    • -r, --run
    • -w, --watch
  • dotnet-test
    • --by-project
    • --ci
    • --coverage
    • -s, --skip-clean
  • github
    • --add-topic <topic>
    • --list-repos
    • --list-topics
    • --remove-topic <topic>
    • -r, --repo <repo>
    • -u, --username <username>
  • install
  • migration
    • -a, --add
    • -d, --delete
    • -r, --run
  • nuget
    • -p, --publish <version>
    • -u, --upgrade
  • webpack
    • -c, --clean
    • -p, --publish
    • -R, --restore
  • webpack-test
    • --ci
    • -c, --clean
    • -R, --restore
  • workspace
    • -c, --clone
    • -f, --fork
    • -u, --usernames <usernames>

@brandongregoryscott brandongregoryscott added the enhancement New feature or request label Nov 4, 2020
@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #130 (7680e9a) into release/2.0.0 (dbfd744) will decrease coverage by 2.18%.
The diff coverage is 79.59%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/2.0.0     #130      +/-   ##
=================================================
- Coverage          77.50%   75.32%   -2.19%     
=================================================
  Files                 28       27       -1     
  Lines               1218     1244      +26     
  Branches             182      243      +61     
=================================================
- Hits                 944      937       -7     
- Misses               239      297      +58     
+ Partials              35       10      -25     
Impacted Files Coverage Δ
src/tests/test-utils.ts 30.48% <37.28%> (ø)
src/modules/echo.ts 21.27% <37.50%> (ø)
src/modules/prompt.ts 35.29% <46.15%> (ø)
src/modules/dotnet-test.ts 44.91% <56.66%> (ø)
src/modules/github.ts 68.89% <67.81%> (ø)
src/modules/formatters.ts 73.33% <73.33%> (ø)
src/modules/git.ts 62.06% <78.57%> (ø)
src/modules/nuget-upgrade.ts 74.13% <80.00%> (ø)
src/modules/file.ts 60.00% <85.71%> (ø)
src/utilities/command-string-builder.ts 85.71% <85.71%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbfd744...0a55fbb. Read the comment docs.

@wintondeshong
Copy link
Contributor

@brandongregoryscott going to try this out this afternoon, but after reading it through it looks awesome! phew, nice work 👏

};

const _installDevAlias = () => {
const _installDevAlias = (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good attention to detail on the install command needing to point to dist. -- in reading this again, I'm thinking we could update this function (in a later PR) to support passing the suffix (ie. dev, etc...). as a maintainer, I keep aliases that point to each of the contributors of this project (ie. and-cli-{github-username}). Wondering if the workspace command could also setup/sync aliases too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be awesome! I'll add an issue to document this

Copy link
Contributor

@wintondeshong wintondeshong left a comment

Choose a reason for hiding this comment

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

@brandongregoryscott knocked out testing a slew of the commands above. I've updated the checklist. I did catch a regression with what I believe is likely happening in the github module. It is likely something simple, but feel it should be investigated before merge.

Totally cool with a beta version 👍 perhaps this branch would end up being merged into a beta branch that we publish off of until we are ready to merge into main.

@@ -0,0 +1,216 @@
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

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

@brandongregoryscott I can't quite spot the breaking change without digging in further, but I am noticing a regression with the workspace command's exiting. No matter the variation (even --help) it hangs and I can't even ctrl+c to exit.

Thing is... it eventually does exit. Just takes several minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird. I've been testing this out this morning and I did not see the slowness with the github command, but I did with the workspace command. I even stripped out almost everything (functions, imports, etc) from that command and it still seems to hang for ~30s to display the help menu... 🤔

const userPrompt = require("./user-prompt");

// #endregion Imports
import { StringUtils } from "andculturecode-javascript-core";
Copy link
Contributor

Choose a reason for hiding this comment

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

So upon further testing of the issue with exits hanging, it appears to be effecting both the github and workspace command series. The common denominator of course is the github module. There is likely something in here causing the exits to hang. It does eventually exit. Just takes several minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for digging into that 👍 I'll take a further look

@brandongregoryscott
Copy link
Contributor Author

brandongregoryscott commented Nov 19, 2020

@wintondeshong I believe (at least part of) the slowdown is from the exports in and-cli.ts. I added those in pretty late in the port which is probably why I didn't notice it. If you comment out all of those, rebuild and run and-cli-dev github, the time goes from ~5s to ~1s:

bscot@BSCOTT-PC ~/acc/fork/brandongregoryscott.AndcultureCode.Cli/ (feature/typescript) -> time and-cli-dev github
...

real    0m4.320s
user    0m0.015s
sys     0m0.015s
bscot@BSCOTT-PC ~/acc/fork/brandongregoryscott.AndcultureCode.Cli/ (feature/typescript) -> time and-cli-dev github
...

real    0m1.130s
user    0m0.015s
sys     0m0.015s

...which sucks. I was hoping to improve the consumer experience by exporting all of the available modules, but maybe there's another approach we can take.

Update: moved all of the exports to index.ts which is the new "main" file for the package. This should allow us to have easy imports + comparable speed to the pre-TS commands

@brandongregoryscott brandongregoryscott changed the base branch from main to release/2.0.0 November 20, 2020 21:12
@wintondeshong
Copy link
Contributor

@wintondeshong I believe (at least part of) the slowdown is from the exports in and-cli.ts. I added those in pretty late in the port which is probably why I didn't notice it. If you comment out all of those, rebuild and run and-cli-dev github, the time goes from ~5s to ~1s:

bscot@BSCOTT-PC ~/acc/fork/brandongregoryscott.AndcultureCode.Cli/ (feature/typescript) -> time and-cli-dev github
...

real    0m4.320s
user    0m0.015s
sys     0m0.015s
bscot@BSCOTT-PC ~/acc/fork/brandongregoryscott.AndcultureCode.Cli/ (feature/typescript) -> time and-cli-dev github
...

real    0m1.130s
user    0m0.015s
sys     0m0.015s

...which sucks. I was hoping to improve the consumer experience by exporting all of the available modules, but maybe there's another approach we can take.

Update: moved all of the exports to index.ts which is the new "main" file for the package. This should allow us to have easy imports + comparable speed to the pre-TS commands

I'm going to step through this here because mine isn't a matter of seconds. It finishes running the actual work from the command (ie. workspace -u wintondeshong) and then hangs for upwards of 30 seconds to a minute.

@brandongregoryscott
Copy link
Contributor Author

I'm going to step through this here because mine isn't a matter of seconds. It finishes running the actual work from the command (ie. workspace -u wintondeshong) and then hangs for upwards of 30 seconds to a minute.

Let me know what you can find. If you have some free time this week, we can try to pair up on it. I have not seen that level of slowness on my Windows machine or Macbook, before or after the export changes.

@wintondeshong
Copy link
Contributor

wintondeshong commented Nov 22, 2020

Here is my current timing...

07:42 /c/Clients/andculturecode  $ time and-cli-brandongregoryscott workspace -u wintondeshong

[and-cli] -----------------------------------------------------------------
[and-cli]
[and-cli] Configuring workspace
[and-cli]
[and-cli] -----------------------------------------------------------------

[and-cli] Synchronizing forks of AndcultureCode for 'wintondeshong'...

[and-cli] Results
[and-cli]  - Successful: 0
[and-cli]  - Unmodified: 24
[and-cli]  - Errored: 0
[and-cli]  - Total: 24



real    1m14.457s
user    0m0.015s
sys     0m0.015s

@wintondeshong
Copy link
Contributor

@brandongregoryscott something seemed up so I re-cloned and did a fresh build of your branch and I'm no longer seeing that performance issue 🤷‍♂️

05:35 /c/Clients/andculturecode  $ time and-cli-brandongregoryscott workspace -u wintondeshong

[and-cli] -----------------------------------------------------------------
[and-cli]
[and-cli] Configuring workspace
[and-cli]
[and-cli] -----------------------------------------------------------------

[and-cli] Synchronizing forks of AndcultureCode for 'wintondeshong'...

[and-cli] Results
[and-cli]  - Successful: 0
[and-cli]  - Unmodified: 24
[and-cli]  - Errored: 0
[and-cli]  - Total: 24



real    0m2.080s
user    0m0.031s
sys     0m0.000s

Copy link
Contributor

@wintondeshong wintondeshong left a comment

Choose a reason for hiding this comment

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

@brandongregoryscott with the performance issue no longer an issue I'm good to approve. I tested the install and nuget --publish commands as well, which leaves basically the deploy series of commands. Recommend getting someone to test those out who might have them in action presently, which would save us time testing.

@brandongregoryscott
Copy link
Contributor Author

@brandongregoryscott something seemed up so I re-cloned and did a fresh build of your branch and I'm no longer seeing that performance issue 🤷‍♂️

So weird 🤔 well, glad it's not an issue!

@brandongregoryscott with the performance issue no longer an issue I'm good to approve. I tested the install and nuget --publish commands as well, which leaves basically the deploy series of commands. Recommend getting someone to test those out who might have them in action presently, which would save us time testing.

Awesome! Thanks for helping out with the testing process. I'll reach out to some of the engineers that are leveraging the remaining deployment commands to see if they have capacity to smoke test the changes.

@brandongregoryscott brandongregoryscott merged commit 5f8b596 into rsm-hcd:release/2.0.0 Dec 1, 2020
@brandongregoryscott brandongregoryscott deleted the feature/typescript branch December 1, 2020 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate feasibility/benefit of porting project to Typescript
2 participants