-
Notifications
You must be signed in to change notification settings - Fork 52
Ensure the project readme is deployed to NPM also #523
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
Conversation
|
Coverage report for commit: 7892f66 Summary - Lines: 100.00% | Methods: 100.00% | Branches: 91.30%
🤖 comment via lucassabreu/comment-coverage-clover |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "integrate": "pnpm format:check && pnpm lint && pnpm test && pnpm build", | ||
| "lint": "eslint --config config/eslint/eslint.config.ts", | ||
| "lint:fix": "pnpm lint --fix", | ||
| "prepublishOnly": "cp .github/README.md ./README.md", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why such a complication and why not put it in the root of the repository as usual? I think the files inside the ".github" directory only relate to the Github configuration and README is needed for documentation regardless of the provider. What don't I know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same discussion we had about the configs, I just prefer as much to not be in the root directory as possible and .github is one of three places it can't be kept, so it's grouped with other related documents. Besides, the pre and post setup is standard to NPM itself and so works well for the use case anyway since it's only an issue for NPM deployments anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this bug highlights that the current solution might be more of a workaround than an improvement and now we’re considering adding an extra hack on top.
According to the GitHub style guide, the README “should be located in the top-level directory for your product or library’s actual codebase”:
https://google.github.io/styleguide/docguide/READMEs.html#what-to-put-in-your-readme
If we move it to the root, contributors and visitors can immediately understand the project without digging into subfolders.
We could technically add a copy command in a prepublishOnly script, but that only runs right before publishing. It wouldn’t help with local development or repo clarity, and it adds unnecessary complexity. Keeping the README in the root is simpler, avoids extra steps and follows convention. Why not keep it simple? 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README is read out of the GitHub directory for the repo and temporarily copied for the NPM deployment. I honestly don't see an issue 😅. I do see your point that the two extra scripts are not needed but I don't think it's an issue to have them and it keeps the root cleaner to me. In saying that, if you insist on it being in the root for conventions sake... maybe. I'm still not convinced but if you insist I suppose I can live with that argument but I still prefer it tidied into the GitHub directory personally since it's a supported location and it's only NPM that needs the deploy scripts which have 0 overhead. Let me know what your opinion is and we can see what to do 🤷🏻♂️.
|
Merged for now. We can discuss moving the README to the root again as a follow up, want to make sure people can actually see the docs on NPM for now though. |
Related Issue
N / A
PR Type
Description
In the last deployment for
v5.0.0-rc.4, the projectREADME.mdwas not deployed since it is now in a different root directory.Proposed Changes
filesin thepackage.jsonto make it mandatoryHow Has This Been Tested?
Tested by running
pnpm publish --tag next --dry-run --no-git-checks.Screenshots/Recordings
N / A
Breaking Changes
Checklist
Additional Notes
N / A