actual
actual copied to clipboard
[WIP] Replace bash scripts with javascript
Would like to hear peoples feedback on this.
- We already use some js scripts, there is one for example that checks migrations
- These should work on any platform and don't require the extra work on windows.
- I still have a couple scripts to migrate
I tried to move as much common functionality into a few util
classes in the top level. Things are a bit more verbose at times, because js isn't necessarily geared towards doing some of the things that bash is good at, and the linter wants things on multiple lines. I tried to keep the LOC count as low as possible at the front.
Other things however are much nicer, for example the previous command used to get the version from the package.json
file is pretty scary looking (to me!)
VERSION=`cat "$ROOT/../../desktop-client/package.json" | grep version | head -n1 | awk -F "\"" '{print $4}' | tr -d '\r\n'`
I've tried to make the final build/watch
scripts as 'user friendly' as possible so its hopefully clear what they're doing exactly.
Deploy Preview for actualbudget ready!
Name | Link |
---|---|
Latest commit | 29f9bb2b0d0be5756c00c813fc7bce8eaf4a6e3e |
Latest deploy log | https://app.netlify.com/sites/actualbudget/deploys/64a466ea3b120c0008ff6c65 |
Deploy Preview | https://deploy-preview-1175--actualbudget.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
π We should definitely NOT do sudo
operations here.
Could you explain more the WHY this would be necessary? What problem does it solve?
It's difficult for me to judge if we need to do these ports because I've not observed any problem with them so far. And I'm not super keen on refactoring something just for the sake of refactors. That carries high risk of breaking things.
π We should definitely NOT do
sudo
operations here.Could you explain more the WHY this would be necessary? What problem does it solve?
It's difficult for me to judge if we need to do these ports because I've not observed any problem with them so far. And I'm not super keen on refactoring something just for the sake of refactors. That carries high risk of breaking things.
I don't want/plan to leave that sudo there, for now I was just trying to see what will make the github builds happy as this works without any of that locally for me on windows and in WSL.
On the why:
On windows, the old scripts only work inside git bash and even then in some cases there is some weird behaviour at times. Its basically why this extra help file needs to exist for windows https://actualbudget.org/docs/contributing/windows
Whereas these new scripts can be run from powershell, command prompt, the VSCode JS debug terminal, or indeed the git bash.
This was basically my only motication behind the change, but in terms of other thoughts I feel like the JS is a bit easier to comprehand, especially if you are not familiar with bash.
I agree that this is a good improvement. (and for clarity, I suggested doing this). As @Shazib said, there are issues contributing on Windows, and I think it would be great to have everything in the repo be JS/TS if possible so people donβt have to learn another language to contribute.
I strongly disagree with this sentiment. Turning 13-line shell scripts into hundreds of lines of javascript code is a big step backwards.
Unless there is a very good reason for doing this I would not want to see this change. So far I'm hearing "Windows issues", but no specifics on 1) what are these issues and 2) why they cannot be solved by changing the existing shell scripts.
I agree leaving them as-is seems best unless we have clear reasons overhauling them is necessary. At work, my coworkers on Windows use the same shell scripts our Linux/macOS user do. I think thereβs some setup to get there, but it doesnβt seem a huge hurdle. Maybe some setup docs could suffice here.
To raise the other side of the ease of contribution argument: if itβs all JS, a bash expert would have to learn JS before contributing π€·π»ββοΈ
Thanks for the discussion guys, its why I opened this in WIP a little early to get feedback!
Turning 13-line shell scripts into hundreds of lines of javascript code is a big step backwards.
One of my goals is for the final scripts to be quite similar is size to the original, if not smaller! Which I think they are quite close. I have had to add some things in the background but if @j-f1's suggestions wrt graceful-fs
etc pan out then this could be reduced further.
To raise the other side of the ease of contribution argument: if itβs all JS, a bash expert would have to learn JS before contributing π€·π»ββοΈ
I'm not sure that's really a valid argument as they'd have to do that anyway unless they planned to contribute only to the build scripts.
In terms of my support for this issue:
- I think it is an entirly valid argument that someone wanting to contribtue to actual will know JS and not bash.
- Some commands just don't exist by default in windows, e.g.
find
orsed
. - Worse still, sometimes these commands can be aliased, for example
ls
in powershell redirects toGet-ChildItem
. These do not always have comparable behaviour so sometimes issues can be a little tricky to track down. - Yes these issues can be solved by following the extra steps and exclusively using the git bash on windows but as someone who normally uses powershell and as the environment set up how i like this isn't a great experience.
- There are portions of these bash scripts that are duplicated, I am generally supportive of refactoring this common logic into one place (e.g. fetching the app version from package.json). We could even unit test it!
- By running scripts in node we are fully platform-agnostic.
Maybe I dont make a strong case, and that ok, I will park further work on this until we have a concensus?
Thanks
Edit: I think Its also a bit easier to debug JS wrt using breakpoints etc
Edit 2: One more: Unified build tools, there are a number of scripts in /bin folders that are already JS, so this approach unifies all build scripts into a single language.
I have incorporated some of the feedback, and hopefully now the LOC is much smaller and we are not adding hundreds of lines of JS.
So I know this is up in the air whether it will be merged but I've been working on it anyway for fun.
The basic idea is that you use the new class BuildScript
to create scripts, and then all your scripts have access to useful properties/methods such as getContentHash()
or desktopRoot
to fetch directory paths. package version numbers etc, instead of you having to manually do ../../desktop-electron
../public/kcab
etc etc.
You may notice BuildScript
times its runs by default so each script will output timing information, we could extend this by adding support for various flags in the class, such as --verbose, --quiet, --log-to-file, and every build script would gain this functionality for the cost of implementing it once.
I'm not sure why the api build is failing - I havent change that command. I will investigate the E2E tests.
Bundle Stats - desktop-client
Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.
As this PR is updated, I'll keep you updated on how the bundle size is impacted.
Total
Files count | Total bundle size | % Changed |
---|---|---|
13 | 2.35 MB | 0% |
Changeset
File | Ξ | Size |
---|---|---|
package.json |
π -4 B (-0.21%) | 1.83 KB -> 1.83 KB |
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
Asset | File Size | % Changed |
---|---|---|
static/js/main.js | 1.05 MB | 0% |
static/js/457.chunk.js | 387.21 KB | 0% |
static/media/Inter.var.woff2 | 317.25 KB | 0% |
static/media/Inter-italic.var.woff2 | 239.29 KB | 0% |
static/media/Inter-roman.var.woff2 | 221.86 KB | 0% |
static/media/bg.svg | 116.73 KB | 0% |
static/js/reports.chunk.js | 20.13 KB | 0% |
static/js/969.chunk.js | 12.94 KB | 0% |
static/js/resize-observer-polyfill.chunk.js | 8.12 KB | 0% |
static/css/main.css | 6.08 KB | 0% |
index.html | 1.68 KB | 0% |
asset-manifest.json | 1.47 KB | 0% |
static/media/browser-server.js | 963 B | 0% |
Bundle Stats - loot-core
Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.
As this PR is updated, I'll keep you updated on how the bundle size is impacted.
Total
Files count | Total bundle size | % Changed |
---|---|---|
2 | 1.97 MB | 0% |
Changeset
No files were changed
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
Asset | File Size | % Changed |
---|---|---|
kcab.worker.js | 1017.19 KB | 0% |
xfo.xfo.kcab.worker.js | 1004.04 KB | 0% |
π Hey! Apologies for taking so long to respond. We've been having internal discussions about this PR.
After much consideration, we think that the existing build scripts should remain in their current shell-script format instead of the proposed refactor to JS.
I do understand that there are some Windows problems with the existing scripts (such as the lack of find
command). We are open to merging a fix for those issues in the existing shell script format.
Hope you understand.