docs: add package.json script guidelines and git hooks guidelines#51
docs: add package.json script guidelines and git hooks guidelines#51rjmunro wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| - Simpler — no `pinst` dependency or pack scripts | ||
| - Only runs during actual development (when building or linting) | ||
| - Doesn't run when installed as library or from git |
There was a problem hiding this comment.
But don't we need to do build the library when installing from git too?
I don't see a proposal for how that would be setup to avoid installing the hook
| **Guidelines:** | ||
|
|
||
| - `clean` removes everything created by `build` (dist folder, generated files) but not data | ||
| - If your tool supports it, `clean --all` should also remove `node_modules` |
There was a problem hiding this comment.
?
Seems like ai wrote a lot of words in this file, without any proposal on how to achieve any of these things.
If this clean --all is wanted, there should be an example of it, which I suspect is going to be a wordy command
| "scripts": { | ||
| "lint:raw": "eslint --ext .ts --ext .js --ignore-pattern dist", | ||
| "lint": "yarn lint:raw .", | ||
| "lint:fix": "yarn lint --fix" |
There was a problem hiding this comment.
If we're talking about what there should be, I've never seen much value in yarn lint:fix script, as the same can be achieved with yarn lint --fix
There was a problem hiding this comment.
Not if it's doing something like:
"lint": "yarn lint:eslint && yarn lint:prettier --check"
"lint:fix": "yarn lint:eslint --fix && yarn lint:prettier --save"
| "test:unit": "jest", | ||
| "test:integration": "jest --config jest.integration.config.js", | ||
| "watch": "jest --watch", | ||
| "cov": "jest --coverage && open-cli coverage/lcov-report/index.html", |
There was a problem hiding this comment.
I would question whether these cov/cov:open should exist, I don't think I've used htem in years and open-cli is just another dependency that I'm not sure we need
The test/watch are maybe a bit confusing/harder to wrangle with vitest as it defaults to watch mode and you have to opt into single run. not a deal breaker, just a question of whether that should affect our standard
| - `start` (in Standard Lifecycle Scripts above) starts in production mode — no hot reload, no source maps | ||
| - Only include `dev` and `start` in projects that have a server component | ||
|
|
||
| Root package.json in workspaces: |
There was a problem hiding this comment.
is this the heading of a section?
| - Use `build:*` and `test:*` to target specific workspaces | ||
| - Keep convenience scripts like `build-sync-local` at workspace level | ||
|
|
||
| ## Blueprint-Specific Scripts |
There was a problem hiding this comment.
I feel like this section shouldnt be here; perhaps a recommendation in the bluepritn-tools is better?
| - These are exceptions to the colon rule for developer convenience | ||
| - Keep these names for consistency with existing documentation | ||
|
|
||
| ## Common Patterns to Avoid |
There was a problem hiding this comment.
This is starting to feel like it is repeating itself now
| ```json | ||
| { | ||
| "scripts": { | ||
| "start": "docusaurus start --port 3030", |
There was a problem hiding this comment.
I'm not sure if I like these markdown files being in this repository like this, (probably shouldnt be loose at the root if they do remain).
rjmunro
force-pushed
the
rjmunro/package-json-guildelines
branch
from
2348865 to
64ff923
Compare
3 participants
About the Contributor
This pull request is posted on behalf of SuperFly.tv.
Type of Contribution
This is a: Documentation improvement
Current Behaviour
There were no written conventions for how
package.jsonscripts should be named and organised across Sofie Automation projects, leading to inconsistency between packages. Similarly, the recommended approach for Husky/git-hooks setup in published packages was not documented.New Behaviour
Adds two guideline documents to this repo:
PACKAGE_SCRIPT_GUIDELINES.md— conventions for naming, namespacing, and organisingpackage.jsonscripts across Sofie packages (lifecycle scripts, colons for namespacing, underscore prefix for internal scripts, etc.)GIT_HOOKS_GUIDELINES.md— documents the options for Husky setup in packages that are also published to npm, and calls out that an RFC is needed to settle the standard approach.Testing Instructions
Read both guideline documents and check they are accurate and consistent with current practice.
Affected areas
Documentation only — no code changes. Adds two new Markdown files to the repo root.
Time Frame
Not urgent. The git hooks section is intentionally left as an open question pending an RFC.
Other Information
The
PACKAGE_SCRIPT_GUIDELINES.mdcontent was aligned with RFC #1699 fromsofie-core. The git hooks document is a placeholder to prompt the RFC discussion.Status