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: npm script awareness, TS fixes, test env refactors #659

Merged
merged 5 commits into from
Nov 2, 2022

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Nov 2, 2022

🧰 Changes

  • We're seeing an issue where TS builds fail if they include this version of rdme in their deps... so this PR installs keyv and it evidently fixes it1 🤷🏽 31c6700
  • npm and yarn use an env var called npm_lifecycle_event to indicate the current npm run script. This feels like a good place to not have our GitHub Action onboarding prompts run 🙅🏽 09cdbf2
  • Small refactors to (hopefully) guarantee that our testbed env logic won't conflict with any env vars in the wild 🦁 6fbfedf
    • All internal testing env values (i.e. for testing CI or testing GitHub Action creation) are prefixed with TEST_RDME_
    • Renamed our NODE_ENV for our tests from test to rdme-test
    • As part of the above, I created a little isTest helper to check whether we're running tests
  • Made small cleanups in our testbed 🧹
    • Optimized our Prettier check to only check Markdown files since ESLint already covers JS/TS files 6a1c232
    • Small cleanup in our test console output b320a97

🧬 QA & Testing

I npm link'd this PR to our Metrics API codebase and ran npm run build in that codebase and confirmed that:

  • The TS error no longer exists
  • Chained npm scripts like the one below work as they previously did:

image

If our test bed passes, we should be good to go 🥳

Footnotes

  1. See: https://stackoverflow.com/q/74141618

- All internal env values (i.e. for testing CI or testing GitHub Action creation) are prefixed with `TEST_RDME_`

- Renamed our `NODE_ENV` for our test from `test` to `rdme-test`

- Created a little `isTest` helper to check whether we're running tests
This isn't used, but we're seeing TS errors when users try to run `tsc` in a repo that contains `rdme` as a dep. Weirdly enough, installing this package fixes it.

See: https://stackoverflow.com/q/74141618
ESLint already covers JS/TS, so the duplicative check is not necessary
@kanadgupta kanadgupta marked this pull request as ready for review November 2, 2022 21:52
@kanadgupta kanadgupta changed the title fix: npm script awareness, test env refactors fix: npm script awareness, TS fixes, test env refactors Nov 2, 2022
@kanadgupta kanadgupta added bug Something isn't working enhancement New feature or request refactor Issues about tackling technical debt GHA / CI Issues specific to GitHub Actions or other CI environments labels Nov 2, 2022
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

that lifecycle env var is a great find.

@kanadgupta kanadgupta merged commit 05c34da into main Nov 2, 2022
@kanadgupta kanadgupta deleted the TS-CI-npm-scripts branch November 2, 2022 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request GHA / CI Issues specific to GitHub Actions or other CI environments refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants