docs: Update CONTRIBUTING_JS.md #776
Open
Conversation
This file contains 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
Updates contributing doc to remove references to obsolete tools, add references to Unified CI and clarify supported platforms
SgtPooki
requested changes
May 24, 2023
Comment on lines
+78
to
+85
| * Node.js Active LTS and the default version of npm that is installed along with it | ||
| * Projects may also support Current LTS | ||
| * Please consult [nodejs.org](https://nodejs.org/) for LTS timeline and for the current Active/LTS version. | ||
| * The latest releases Chromium, FireFox and WebKit | ||
| * Electron | ||
| * Main process only, latest release | ||
|
|
||
| ```js | ||
| class NotFoundError extends Error { | ||
| constructor (message) { | ||
| super(message || 'Resource was not found') | ||
| this.name = 'NotFoundError' | ||
| this.code = NotFoundError.code | ||
| } | ||
| } | ||
|
|
||
| NotFoundError.code = 'ERR_NOT_FOUND' | ||
| exports.NotFoundError = NotFoundError | ||
| ``` | ||
| We do not go out of our way to break compatibility with platforms, but we can only test on the above. Where we require a recently released feature (such as an encryption algorithm or browser API) this will be noted by the [engines](https://docs.npmjs.com/cli/v9/configuring-npm/package-json#engines) field of the `package.json`. |
| - **Scope** - The scope could be anything specifying the place of the commit change. For example `api`, `cli`, etc... | ||
| - **Breaking Changes** - Should be identified at the end of commit message. Start with the words `BREAKING CHANGE:` on a new line followed by a space or two new lines. The rest of the commit message is then used to describe in detail what was broken and the migration path (if there is one). | ||
| - **Scope** - An optional scope can be added specifying the place of the commit change. For example `api`, `cli`, etc... | ||
| - **Breaking Changes** - Should be identified by appending a `!` to the end of the type (e.g. `feat!: ...`) and at the end of commit message. Start with the words `BREAKING CHANGE:` on a new line followed by a space or two new lines. The rest of the commit message is then used to describe in detail what was broken and the migration path (if there is one). This will appear in the generated release notes. |
There was a problem hiding this comment.
Suggested change
| - **Breaking Changes** - Should be identified by appending a `!` to the end of the type (e.g. `feat!: ...`) and at the end of commit message. Start with the words `BREAKING CHANGE:` on a new line followed by a space or two new lines. The rest of the commit message is then used to describe in detail what was broken and the migration path (if there is one). This will appear in the generated release notes. | |
| - **Breaking Changes** - Should be identified by appending a `!` to the end of the type (e.g. `feat!: ...`). Start with the words `BREAKING CHANGE:` on a new line followed by a space or two new lines. The rest of the commit message is then used to describe in detail what was broken and the migration path (if there is one). This will appear in the generated release notes. |
| "coverage-publish": "aegir coverage publish" | ||
| } | ||
| } | ||
| > aegir docs | ||
| ``` | ||
|
|
||
| You also need to add it your `devDependencies` by running: |
There was a problem hiding this comment.
this should probably be higher, considering the section is "setting up aegir"
|
|
||
| We want to see the web move forward, and some of us enjoy writing their JavaScript with things like `const` and arrow functions. | ||
| We've also found that having types means there's less magic in our codebases since it becomes harder to do use the extreme ends of JavaScript's flexibility which then makes everything easier to follow, lowering cognitive overhead and maintenance burden. |
There was a problem hiding this comment.
Suggested change
| We've also found that having types means there's less magic in our codebases since it becomes harder to do use the extreme ends of JavaScript's flexibility which then makes everything easier to follow, lowering cognitive overhead and maintenance burden. | |
| We've also found that having types means there's less magic in our codebases since it becomes harder to use the extreme ends of JavaScript's flexibility, making everything easier to follow, and lowering cognitive overhead and maintenance burden. |
Comment on lines
+286
to
288
| #### Do I have to bundle everything with esbuild? | ||
|
|
||
| No. But other people might ask you to at some point, so it may be better to be prepared. |
There was a problem hiding this comment.
aegir+esbuild has proven troublesome for use with front-end projects and component-lib packages (e.g. ipld-explorer-components).
it would be really nice to have some examples and/or direction so we can say aligned with @ipfs/helia-dev best practices.
There was a problem hiding this comment.
I don't think we have any right now because aegir is mostly used for library-style modules - maybe you could write some?
Would it mean choosing a frontend framework for the user? Sounds fraught with peril and/or almost immediate obsolescence.
There was a problem hiding this comment.
Not choosing a frontend framework at all, but choosing a standard bundler and config. We already have standard for style, mocha+playwright for testing, and esbuild for isomorphic non-frontend libs.
Choosing webpack/vite/rollup/snowpack or similar for client-side libraries and webpages, advising against react-scripts, would benefit us in the long run.
It would be great to bundle this into aegir, but maybe an extension of aegir would be a better call.
BigLep
reviewed
May 24, 2023
There was a problem hiding this comment.
Thanks for updating this. It's a good/useful doc.
Other things coming to mind: conventions around docs for generating table of contents. Maybe point at suggested extensions to help do this.
I left some other comments, but feel free to merge without me looking again.
| * Node.js Active LTS and the default version of npm that is installed along with it | ||
| * Projects may also support Current LTS | ||
| * Please consult [nodejs.org](https://nodejs.org/) for LTS timeline and for the current Active/LTS version. | ||
| * The latest releases Chromium, FireFox and WebKit |
There was a problem hiding this comment.
Suggested change
| * The latest releases Chromium, FireFox and WebKit | |
| * The latest releases of "desktop" Chromium, FireFox and WebKit |
| * Please consult [nodejs.org](https://nodejs.org/) for LTS timeline and for the current Active/LTS version. | ||
| * The latest releases Chromium, FireFox and WebKit | ||
| * Electron | ||
| * Main process only, latest release | ||
|
|
There was a problem hiding this comment.
Make a statement about "mobile" support and testing?
|
|
||
| #### Testing | ||
|
|
||
| Since `js-ipfs` is meant to be both a Node.js and Browser app, we strongly recommend having tests that run in both platforms, always. For most cases, we use [mocha](http://mochajs.org) to run write the tests and [karma](http://karma-runner.github.io) to automate the test execution in the browser. This solution has been extremely convenient. | ||
| Since our modules are meant to be isomorphic as far as possible, we strongly recommend having tests that run in all supported platforms, always. For most cases, we use [mocha](http://mochajs.org) to run write the tests and [playwright-test](https://www.npmjs.com/package/playwright-test) to automate the test execution in browsers and [electron-mocha](https://www.npmjs.com/package/electron-mocha) to run them in Electron. This solution has been extremely convenient. |
There was a problem hiding this comment.
Suggested change
| Since our modules are meant to be isomorphic as far as possible, we strongly recommend having tests that run in all supported platforms, always. For most cases, we use [mocha](http://mochajs.org) to run write the tests and [playwright-test](https://www.npmjs.com/package/playwright-test) to automate the test execution in browsers and [electron-mocha](https://www.npmjs.com/package/electron-mocha) to run them in Electron. This solution has been extremely convenient. | |
| Since our modules are meant to be isomorphic as far as possible, we strongly recommend having tests that run in all supported platforms, always. For most cases, we use: | |
| * [mocha](http://mochajs.org) to run write the tests | |
| * [playwright-test](https://www.npmjs.com/package/playwright-test) to automate the test execution in browsers | |
| * [electron-mocha](https://www.npmjs.com/package/electron-mocha) to run them in Electron. | |
| This solution has been extremely convenient. |
|
|
||
| #### Releasing | ||
|
|
||
| Each time a new release happens, these are the steps we follow to make sure nothing gets left out: | ||
| Releases should be automated and occur at the end of a successful CI run on the default branch of the project. |
There was a problem hiding this comment.
Is that true? I thought we often are batching up changes before we do a release.
|
|
||
| #### Testing | ||
|
|
||
| Since `js-ipfs` is meant to be both a Node.js and Browser app, we strongly recommend having tests that run in both platforms, always. For most cases, we use [mocha](http://mochajs.org) to run write the tests and [karma](http://karma-runner.github.io) to automate the test execution in the browser. This solution has been extremely convenient. | ||
| Since our modules are meant to be isomorphic as far as possible, we strongly recommend having tests that run in all supported platforms, always. For most cases, we use [mocha](http://mochajs.org) to run write the tests and [playwright-test](https://www.npmjs.com/package/playwright-test) to automate the test execution in browsers and [electron-mocha](https://www.npmjs.com/package/electron-mocha) to run them in Electron. This solution has been extremely convenient. | ||
|
|
||
| #### Releasing |
There was a problem hiding this comment.
Are there more statements we can make about release notes, changelogs, etc ?
A couple of observations:
- It doesn't look like Helia's has been getting updated: https://github.com/ipfs/helia/blob/main/CHANGELOG.md
- I like a lot of the principles in https://keepachangelog.com/en/1.0.0/ . I like that we automate a lot of the changelog content. I think the part we're often missing is some small editorializing on "why you might care about this release". I dodn't think that necessarily comes through with our short bullets of PR titles.
|
|
||
| #### Documentation | ||
|
|
||
| Documentation will be generated automatically by the JSDoc based TS types in the codebase. | ||
| Typed ESM projects will have documentation generated automatically from JSDoc comments in the codebase, TypeScript projects will accomplish the same thing by using the types directly. |
There was a problem hiding this comment.
Suggested change
| Typed ESM projects will have documentation generated automatically from JSDoc comments in the codebase, TypeScript projects will accomplish the same thing by using the types directly. | |
| Typed ESM projects will have documentation generated automatically from JSDoc comments in the codebase; TypeScript projects will accomplish the same thing by using the types directly. |
|
|
||
| Type definitions and type imports should be created on the top of any JS file (below eventual requires needed). For tooling instructions and best practices, see [Documentation for JSDoc based TS types](https://github.com/ipfs/aegir/blob/master/md/ts-jsdoc.md). | ||
| A `gh-pages` branch will be created and this should be selected to be published via the settings your GitHub project under `General > Pages > Build and deployment > Branch`. |
There was a problem hiding this comment.
Suggested change
| A `gh-pages` branch will be created and this should be selected to be published via the settings your GitHub project under `General > Pages > Build and deployment > Branch`. | |
| A `gh-pages` branch will be created, and this should be published to via the GitHub project settings GitHub under `General > Pages > Build and deployment > Branch`. |
We say this "should be" done, but we don't explain why this is important. This is where aegir will commit API docs?
| - **refactor**: A code change that neither fixes a bug nor adds a feature | ||
| - **perf**: A code change that improves performance | ||
| - **test**: Adding missing tests | ||
| - **chore**: Changes to the build process or auxiliary tools and libraries such as documentation generation | ||
| - **Scope** - The scope could be anything specifying the place of the commit change. For example `api`, `cli`, etc... | ||
| - **Breaking Changes** - Should be identified at the end of commit message. Start with the words `BREAKING CHANGE:` on a new line followed by a space or two new lines. The rest of the commit message is then used to describe in detail what was broken and the migration path (if there is one). | ||
| - **Scope** - An optional scope can be added specifying the place of the commit change. For example `api`, `cli`, etc... |
There was a problem hiding this comment.
Suggested change
| - **Scope** - An optional scope can be added specifying the place of the commit change. For example `api`, `cli`, etc... | |
| - **Scope** - An optional scope can be added in parentheses specifying the place of the commit change. For example `api`, `cli`, etc... |
| @@ -202,35 +169,19 @@ We've created [a module](https://github.com/ipfs/aegir) to help us achieve all o | |||
|
|
|||
| ##### Setting up `aegir` | |||
|
|
|||
| There are a couple of binaries that `aegir` provides for you to use | |||
| `aegir` provides several commands for you to use | |||
There was a problem hiding this comment.
Suggested change
| `aegir` provides several commands for you to use | |
| [`aegir`](https://github.com/ipfs/aegir) provides several commands for you to use |
| "release": "aegir release", | ||
| "docs": "aegir docs" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ##### `.gitignore` |
There was a problem hiding this comment.
The .gitignore link below didn't resolve
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updates contributing doc to remove references to obsolete tools, add references to Unified CI and clarify supported platforms
Refs: ipfs/helia#113