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

Convert to TS project #81

Merged
merged 18 commits into from
May 28, 2023
Merged

Convert to TS project #81

merged 18 commits into from
May 28, 2023

Conversation

spenserblack
Copy link
Owner

Closes #80

@spenserblack spenserblack linked an issue May 22, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@d1c7c54). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main       #81   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         2           
  Lines           ?        31           
  Branches        ?         3           
========================================
  Hits            ?        31           
  Misses          ?         0           
  Partials        ?         0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jcbhmr
Copy link
Contributor

jcbhmr commented May 23, 2023

If you don't want to do the song and dance to validate the committed dist/ folder, you can use something that automagically does it for you on release tags!

https://github.com/JasonEtco/build-and-tag-action

name: Publish

on:
  release:
    types: [published, edited]

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v2
        with:
          ref: ${{ github.event.release.tag_name }}
      - name: Install deps and build
        run: npm ci && npm run build
      - uses: JasonEtco/build-and-tag-action@v2
        env:
          GITHUB_TOKEN: ${{ github.token }}

The guide to JavaScript Actions recommends including node_modules in your repository, and manual steps to following the versioning recommendations. There are anti-patterns there that just don't sit right with me; so we can enable the same workflow, automatically!

I haven't used it (I haven't made a JS action myself) but it seems interesting!

@spenserblack
Copy link
Owner Author

I'm checking that dist/ is built on every PR/push so that users can use @main for the most up-to-date version. And I could generate a commit for every push to main, but I'd rather have everything bundled in a single commit 🙂
Right now it's not too much trouble, but maybe I'll add a workflow specific to PRs to help out contributors that forget this step.

@jcbhmr jcbhmr mentioned this pull request May 24, 2023
5 tasks
@spenserblack
Copy link
Owner Author

@jcbhmr FYI if you want to test this action before I merge this rewrite. I don't really have time to test it out right now.

@spenserblack spenserblack marked this pull request as ready for review May 24, 2023 19:52
@spenserblack
Copy link
Owner Author

BTW feel free to make PRs targeting the 80/rewrite branch if you have any ideas prior to merge.

}
},
// https://webinstall.dev/
"postCreateCommand": "curl -sS https://webi.sh/shfmt | sh && curl -sS https://webi.sh/shellcheck | sh"
"postCreateCommand": ".devcontainer/postCreate.sh",
Copy link
Contributor

@jcbhmr jcbhmr May 28, 2023

Choose a reason for hiding this comment

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

-  "postCreateCommand": ".devcontainer/postCreate.sh",
+  "postCreateCommand": "pnpm install",

It is just one command after all 😂

Copy link
Owner Author

Choose a reason for hiding this comment

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

Just a bit of future-proofing for when more post-create commands need to be added. Plus, it's a cheat to add a bit more language diversity to this project 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I (personally since I wrote it 😁) think that the idea of using gh repo clone into a tmp folder and using $GIT_DIR and $GIT_WORK_TREE is better than trying to finagle the git clone into the working tree.

Pros of gh repo clone user/repo.wiki /tmp/123abc:

  1. You don't deal with #72
  2. You don't need to worry about the .git folder in the $PWD subtree affecting anything else (git submodule weirdness detection)
  3. You can clone the remote repo directly instead of git init and fetch

Remember I am biased. 😜

Copy link
Owner Author

Choose a reason for hiding this comment

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

Right now this PR is an attempt to translate to a different language without much change to the implementation. This is partly my obsession with "clean" git histories, but I'd want this rewrite to have few to no significant side effects. But I'm definitely open to changing the implementation after this PR 🙂

@@ -0,0 +1,76 @@
import * as core from "@actions/core";
import { exec } from "@actions/exec";
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool 😎 library that makes it much easier to do shell stuff: https://github.com/google/zx#readme
You don't have to use it, I'm just letting you know this cool thing exists 😊

Copy link
Owner Author

Choose a reason for hiding this comment

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

I never bothered researching this, but I've always wondered why @actions/exec exists when there are already other popular implementations for calling commands (another one is execa). Like, is there something about @actions/exec that optimizes it for actions? All I've found is actions/toolkit#315

"main": "lib/index.js",
"scripts": {
"build": "tsc",
"format": "prettier --write .",
Copy link
Contributor

Choose a reason for hiding this comment

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

-    "format": "prettier --write .",
+    "format": "prettier --ignore-path .gitignore -w .",

This would make it so you don't need a .prettierignore since your .gitignore would double as your .prettier ignore.
Though then again this might conflict with the fact that there's minified JS in dist/ in source control 🤦‍♀️

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, prettifying built files is an annoyance that I'd like to avoid.

.gitignore Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to use https://github.com/github/gitignore/blob/main/Node.gitignore since it's "official" but that's just me 😊

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, besides the fact that it ignores dist/ by default AFAIK, there's no reason I'm not using it besides being to lazy to fetch that file 😆

src/has-wiki.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default gh repo clone octocat/project.wiki has a good enough error message, right? 🤔

$ gh repo clone nodejs/node.wiki
The 'nodejs/node' repository does not have a wiki

I think just letting that gh repo clone command fail and print its own error message is OK. 🤷‍♀️

Copy link
Owner Author

Choose a reason for hiding this comment

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

TIL that gh has an explicit check if you're cloning a wiki 😆

While gh is an awesome tool, one of the things I want to do is drop dependency on gh and implement as much with pure TS as possible.

Comment on lines +23 to +24
const baseOptions = { cwd };
await exec("git", ["init"], baseOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember! This still has the issue with the existing .git folder from the main repo that was actions/checkout-ed into here! #72

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yup! This will be addressed in a subsequent change.

@jcbhmr
Copy link
Contributor

jcbhmr commented May 28, 2023

Just noticed that the test.yml file was removed 😬 I highly recommend that that e2e test stays in there! It's arguably the most important test of all and has saved me a couple of times. https://github.com/spenserblack/actions-wiki/actions/workflows/test.yml

image

@spenserblack spenserblack merged commit 7f81269 into main May 28, 2023
@spenserblack spenserblack deleted the 80/rewrite branch May 28, 2023 17:33
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.

Rewrite in TS/JS
2 participants