-
-
Notifications
You must be signed in to change notification settings - Fork 2k
docs: Update CONTRIBUTING.md #4886
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,43 +74,52 @@ Unique to Preact we do support several ways to hook into our renderer. All our a | |
|
|
||
| ## Important Branches | ||
|
|
||
| We merge every PR into the `main` branch which is the one that we'll use to publish code to npm. For the previous Preact release line we have a branch called `8` which is in maintenance mode. As a new contributor you won't have to deal with that ;) | ||
| We have a couple of important branches to be aware of: | ||
|
|
||
| - `main` - This is the main development branch and represents the upcoming v11 release line. | ||
| - `v10.x` - This branch represents the current stable release line, v10. | ||
|
|
||
| As we have yet to release v11, contributors are welcome to use either branch to build upon. We will try to port changes between the branches when possible, to keep them in sync, but if you're feeling generous, we'd love if you'd submit PRs to both branches! | ||
|
|
||
| ## Creating your first Pull-Request | ||
|
|
||
| We try to make it as easy as possible to contribute to Preact and make heavy use of GitHub's "Draft PR" feature which tags Pull-Requests (short = PR) as work in progress. PRs tend to be published as soon as there is an idea that the developer deems worthwhile to include into Preact and has written some rough code. The PR doesn't have to be perfect or anything really ;) | ||
|
|
||
| Once a PR or a Draft PR has been created our community typically joins the discussion about the proposed change. Sometimes that includes ideas for test cases or even different ways to go about implementing a feature. Often this also includes ideas on how to make the code smaller. We usually refer to the latter as "code-golfing" or just "golfing". | ||
|
|
||
| When everything is good to go someone will approve the PR and the changes will be merged into the `main` branch and we usually cut a release a few days/ a week later. | ||
| When everything is good to go someone will approve the PR and the changes will be merged into the `main` or `v10.x` branches and we usually cut a release a few days/ a week later. | ||
|
|
||
| _The big takeaway for you here is, that we will guide you along the way. We're here to help to make a PR ready for approval!_ | ||
|
|
||
| The short summary is: | ||
|
|
||
| 1. Make changes and submit a PR | ||
| 2. Modify change according to feedback (if there is any) | ||
| 3. PR will be merged into `main` | ||
| 3. PR will be merged into `main` or `v10.x` | ||
| 4. A new release will be cut (every 2-3 weeks). | ||
|
|
||
| ## Commonly used scripts for contributions | ||
|
|
||
| Scripts can be executed via `npm run [script]` or `yarn [script]` respectively. | ||
| Scripts can be executed via `npm run [script]`. | ||
|
|
||
| - `build` - compiles all packages ready for publishing to npm | ||
| - `build:core` - builds just Preact itself | ||
| - `build:debug` - builds the debug addon only | ||
| - `build:devtools` - builds the devtools addon only | ||
| - `build:hooks` - builds the hook addon only | ||
| - `build:test-utils` - builds the test-utils addon only | ||
| - `build:compat` - builds the compat addon only | ||
| - `build:jsx` - builds the JSX runtime addon only | ||
| - `test` - Run all tests (linting, TypeScript definitions, unit/integration tests) | ||
| - `test:ts` - Run all tests for TypeScript definitions | ||
| - `test:karma` - Run all unit/integration tests. | ||
| - `test:karma:watch` - Same as above, but it will automatically re-run the test suite if a code change was detected. | ||
| - `test:vitest` - Run all unit/integration tests. | ||
| - `test:vitest:watch` - Same as above, but it will automatically re-run the test suite if a code change was detected. | ||
|
|
||
| But to be fair, the only ones we use ourselves are `build` and `test:karma:watch`. The other ones are mainly used on our CI pipeline and we rarely use them. | ||
| But to be fair, the ones we mostly use locally are `build` and `test:vitest:watch`. The other ones are mainly used on our CI pipeline. | ||
|
|
||
| _Note: Both `test:karma` and `test:karma:watch` listen to the environment variable `COVERAGE=true`. Disabling code coverage can significantly speed up the time it takes to complete the test suite._ | ||
| _Note: Both `test:vitest` and `test:vitest:watch` listen to the environment variable `COVERAGE=true`. Disabling code coverage can significantly speed up the time it takes to complete the test suite._ | ||
|
|
||
| _Note2: The test suite is based on `karma` and `mocha`. Individual tests can be executed by appending `.only`:_ | ||
| _Note2: Individual tests can be executed by appending `.only`:_ | ||
|
|
||
| ```jsx | ||
| it.only('should test something', () => { | ||
|
|
@@ -167,16 +176,6 @@ Check out the [official TypeScript documentation](https://www.typescriptlang.org | |
|
|
||
| _Note that we have separate tests for our TypeScript definition files. We only use `ts-check` for local development and don't check it anywhere else like on the CI._ | ||
|
|
||
| ### Why does the code base often use `let` instead of `const`? | ||
|
|
||
| There is no real reason for that other a historical one. Back before auto-formatting via prettier was a thing and minifiers weren't as advanced as they are today we used a pretty terse code-style. The code-style deliberately was aimed at making code look as concise and short as possible. The `let` keyword is a bit shorter than `const` to write, so we only used that. This was done only for stylistic reasons. | ||
|
|
||
| This helped our minds to not lose sight of focusing on size, but made it difficult for newcomers to start contributing to Preact. For that reason alone we switched to `prettier` and loosened our rule regarding usage of `let` or `const`. Today we use both, but you can still find many existing places where `let` is still in use. | ||
|
|
||
| In the end there is no effect on size regardless if you use `const`, `let` or use both. Our code is downtranspiled to `ES5` for npm so both will be replaced with `var` anyways. Therefore it doesn't really matter at all which one is used in our codebase. | ||
|
|
||
| This will only become important once shipping modern JavaScript code on npm becomes a thing and bundlers follow suit. | ||
|
|
||
|
Comment on lines
-170
to
-179
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we have too many weird Can revert if we want it for the "Our code is downtranspiled to |
||
| ## How to create a good bug report | ||
|
|
||
| To be able to fix issues we need to see them on our machine. This is only possible when we can reproduce the error. The easiest way to do that is narrow down the problem to specific components or combination of them. This can be done by removing as much unrelated code as possible. | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Verbiage recommendations welcome, if anyone has them. Not sure I love this line as-is.