actual icon indicating copy to clipboard operation
actual copied to clipboard

[WIP] Replace bash scripts with javascript

Open Shazib opened this issue 1 year ago β€’ 8 comments

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.

Shazib avatar Jun 23 '23 23:06 Shazib

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jun 23 '23 23:06 netlify[bot]

πŸ‘‹ 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.

MatissJanis avatar Jun 24 '23 14:06 MatissJanis

πŸ‘‹ 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.

Shazib avatar Jun 25 '23 02:06 Shazib

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.

j-f1 avatar Jun 25 '23 12:06 j-f1

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.

MatissJanis avatar Jun 25 '23 16:06 MatissJanis

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 πŸ€·πŸ»β€β™‚οΈ

trevdor avatar Jun 25 '23 16:06 trevdor

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 or sed.
  • Worse still, sometimes these commands can be aliased, for example ls in powershell redirects to Get-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.

Shazib avatar Jun 25 '23 19:06 Shazib

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.

Shazib avatar Jun 26 '23 21:06 Shazib

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.

Shazib avatar Jun 29 '23 00:06 Shazib

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%

github-actions[bot] avatar Jul 04 '23 18:07 github-actions[bot]

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%

github-actions[bot] avatar Jul 04 '23 18:07 github-actions[bot]

πŸ‘‹ 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.

MatissJanis avatar Jul 05 '23 07:07 MatissJanis