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 #5184, ensure Typescript developer experience doesn't influence user experience #5185

Merged
merged 2 commits into from
May 17, 2022

Conversation

code-ape
Copy link
Collaborator

@code-ape code-ape commented May 17, 2022

Related PRs and Issues

  1. master branch build broken: 'npm i knex/knex@master' does not work #5184
  2. Improving Typescript developer experience #5151

Reason

#5151 caused all end user installs of Knex to fail due to single line in package.json. Intention was that when developers ran npm install while working on Knex it would automatically build the Typescript files. This was a mistake and it should be a Husky script for the git hook post-checkout.

Changes

  1. Converted automation to build Typescript files from npm postInstall and prepublishOnly hooks to prepare hook in package.json.
  2. Added Husky git hook to automatically build Typescript files on branch changes in package.json.
  3. Fixed files section to properly exclude non-publishable lib/ files in package.json.
  4. Fixed npm command build:gitignore to work on Windows in package.json.
  5. Updated contributing guide to mirror above changes in CONTRIBUTING.md.
  6. Added Github Actions CI integration test for installing the package on all combinations of OS and Node version in .github/workflows/integration-tests.yml.
  7. Updated Husky dependency from 7.x to 8.x in package.json.
  8. Updated Husky pre-commit hook with newer shell template in .husky/pre-commit.
  9. Added entry to ignore packed Knex package tar gzip files in .gitignore.

@code-ape
Copy link
Collaborator Author

code-ape commented May 17, 2022

@kibertoad or @OlivierCavadenti please have all tests run and if they pass I'll update description and convert from draft for review!

@code-ape code-ape changed the title Fix #5148, ensure Typescript developer experience doesn't influence user experience Fix #5184, ensure Typescript developer experience doesn't influence user experience May 17, 2022
@kibertoad
Copy link
Collaborator

@code-ape integration tests got nuked somehow

@code-ape
Copy link
Collaborator Author

code-ape commented May 17, 2022

@kibertoad it's because I'm an outside contributor and changed that CI file. I think there should be some way for you to "approve and run it"?

Correction: It's because I messed up the YAML 😅

@code-ape code-ape closed this May 17, 2022
@code-ape code-ape reopened this May 17, 2022
@coveralls
Copy link

coveralls commented May 17, 2022

Coverage Status

Coverage remained the same at 92.233% when pulling 93ee7aa on code-ape:I-5184/move-auto-ts-build-to-husky into a4df3e3 on knex:master.

@kibertoad
Copy link
Collaborator

@code-ape I think restriction only applies to first-time committers, CI should kick-in automatically for you.

@code-ape code-ape force-pushed the I-5184/move-auto-ts-build-to-husky branch from f98a5f8 to fe6bbd4 Compare May 17, 2022 17:13
@code-ape code-ape force-pushed the I-5184/move-auto-ts-build-to-husky branch from cef0dce to 93ee7aa Compare May 17, 2022 17:29
@code-ape code-ape marked this pull request as ready for review May 17, 2022 17:44
@code-ape
Copy link
Collaborator Author

@kibertoad and @OlivierCavadenti, draft status removed, description updated to describe all changes, and all CI tests passing. Handing it off to you to do as you will 😄

@kibertoad kibertoad merged commit 211c6a7 into knex:master May 17, 2022
@code-ape code-ape deleted the I-5184/move-auto-ts-build-to-husky branch May 17, 2022 21:38
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.

4 participants