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

Adding some contribution guidelines #277

Merged
merged 8 commits into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs-sidebar.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ const sidebars = {
items: [
'contributing/project-details/database',
'contributing/project-details/architecture',
'contributing/project-details/electron',
'contributing/project-details/migrations',
'contributing/project-details/advice',
],
},
'contributing/preview-builds',
Expand Down
5 changes: 5 additions & 0 deletions docs/contributing/project-details/advice.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Important Advice

* Any changes made to the `global.Actual` object must happen inside the respective electron and browser preload scripts. Whilst re-assigning items will work in the browser it is not supported in electron.

* Similarly, and changes made to `global.Actual` should be manually tested on the electron builds as well as the browser.
7 changes: 0 additions & 7 deletions docs/contributing/project-details/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,3 @@ When actual runs, it runs the front-end react based web app, as well as a local
In the web app, this background server runs in a [web worker](https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers), and in the electron app it spins up a [background process](https://nodejs.org/dist/latest-v16.x/docs/api/child_process.html#child_processforkmodulepath-args-options) which communicates over [WebSockets](https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API).

The code which is used by this background server, as well as code which is shared across the web app and desktop versions of actual typically lives inside the `loot-core` package.

### Electron Notes

* Generally speaking, it is unlikely that features/fixes you contribute to actual will require electron-specific changes. If you think that is likely feel free to discuss on github or in the actual discord.

* Details of the motivation behind the usage of WebSockets in the electron app can be found in the [Pull Request](https://github.com/actualbudget/actual/pull/1003) where the changes were made.

10 changes: 10 additions & 0 deletions docs/contributing/project-details/electron.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Electron Notes

* Generally speaking, it is unlikely that features/fixes you contribute to actual will require electron-specific changes. If you think that is likely feel free to discuss on github or in the actual discord.

* Details of the motivation behind the usage of WebSockets in the electron app can be found in the [Pull Request](https://github.com/actualbudget/actual/pull/1003) where the changes were made.

* Due to Electron security requirements there are some restrictions on what can be passed from front-end to (local) back-end. Generally limited to strings/ints via the `ipcRenderer`

* Making changes to the `global.Actual` object MUST happen inside the preload script. Due to electron security requirements this object is siloed and can only pass messages via `ipcRenderer`

15 changes: 15 additions & 0 deletions docs/contributing/project-details/migrations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# DB Migrations Guide
Shazib marked this conversation as resolved.
Show resolved Hide resolved

There are some important considerations to make when adding a feature with a db migration.

* DB Migrations also require publishing a new API version as the migrations also need to be applied there.

* The AQL Schema file will likely need to be updated to match any table changes.

* You must place your migration file in the `loot-core/migrations` folder, with a strict naming convention.

* The naming convention is as follows: `TIMESTAMP_name.sql`. for example. `1694438752000_add_goal_targets.sql`

* It is strongly discouraged to try to remove columns and tables, This makes reverting changes impossible and introduces unneccessary risk when we can simply stop using them in code.
Fixed Show fixed Hide fixed

* You should be very deliberate with your migration. When adding a feature, try to think about future scenarios and options that may be desired later, so we can minimise the number of migrations.
Loading