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

Refactor/scripts standalone #2035

Merged
merged 23 commits into from
Aug 25, 2023
Merged

Refactor/scripts standalone #2035

merged 23 commits into from
Aug 25, 2023

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Aug 17, 2023

PR Checklist

  • PR title descriptive (can be used in release notes)

Description

In order to develop a desktop app, we need to be able to build and package the backend in a way that is consumable.

Main changes

  • Add tsup to allow compilation of scripts workspace using esbuild toolchain
  • Add build script to package scripts workspace
  • Add bin folder and bindings to allow executing scripts from compiled bundle (instead of compiling from typescript at runtime).
  • Refactor shared package to be consumable as part of build processes
  • Refactor additional packages to allow build from scripts

Additional Changes

  • Improve and update scripts readme
  • Fix ts-node-dev implementation

Review Notes

  • For now the yarn scripts and yarn workflow should continue to work exactly as before (will be deprecated in the future following point above)

  • The new commands yarn app-scripts or yarn app-workflow should auto-build the scripts library and then execute as above. It should only rebuild when the scripts workspace package.json version updates (can test manually). Commands executed through this should then work as above, with initial execution being significantly faster (for me change from around 7s -> 2s)

Dev Notes

For now the commands yarn scripts and yarn app-scripts should be interchangeable, where the former executes the scripts in the usual way (compiling typescript files via ts-node at runtime) and the later executes from the built bundle.

I had attempted integrating NX to allow automated caching and rebuilding scripts as required, although became unstuck due to an issue with executing interactive scripts from nx (https://github.com/nrwl/nx/issues/8269 ). So I reverted the changes and may revisit in the future (have closed #1695 accordingly). Instead I opted to manually include the package.json in build and compare version numbers from src/dist folders. This means when updating scripts in the future we should also remember to bump the scripts package.json version

Follow-up Tasks (to be addressed in PR #2061)

  • Part of standalone build still triggers spawnsync commands which requires full dev environment (e.g. typescript, installed packages etc.). Further work required to make linked packages consumable (either by building into npm package or updating package.json to export src index.ts and putting consumable methods in related index for use. Similarly calling spawnsync should be replaced with worker threads
  • Reconsider NX integration (likely when looking into desktop app build scripts)
  • Replace direct scripts ts-node invocation method (currently yarn scripts) in favour of compiled bin execution (yarn app-scripts)
  • Documentation
  • Integrate into desktop electron app (https://github.com/bennymeg/nx-electron)

Git Issues

Closes #

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

This was referenced Aug 20, 2023
@chrismclarke chrismclarke mentioned this pull request Aug 23, 2023
11 tasks
@chrismclarke chrismclarke marked this pull request as ready for review August 23, 2023 18:19
@chrismclarke chrismclarke mentioned this pull request Aug 23, 2023
14 tasks
Copy link
Collaborator

@esmeetewinkel esmeetewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn app-workflow sync on the plh_tz and debug deployments is giving an error at the populate_src_assets step:

Error: Unknown arguments: open, input-folder, output-folder, cache-folder

(N.B. yarn workflow sync runs without problem)
image

yarn scripts version and yarn app-scripts version both working as expected
yarn workflow deployment set and yarn app-workflow deployment set both working as expected

Also this branch seems to conflict with master in the yarn.lock file, could you resolve that?

@chrismclarke
Copy link
Member Author

Thanks @esmeetewinkel, I think the issue is coming from the way some workflows need to call other workflows, and in the case of app-data processing not all the code required was available and it actually was just trying to execute the wrong code instead (super strange).

Most of these issues I had marked for follow-up in #2061, although I've taken a quick look and should have fixed the specific sync workflow.

I've also updated the scripts version number, so assuming the auto-build feature is working correctly you should be able to just try rerun yarn app-workflow sync and everything just work this time round (if not let me know).

@esmeetewinkel esmeetewinkel self-requested a review August 25, 2023 08:38
Copy link
Collaborator

@esmeetewinkel esmeetewinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming the auto-build feature is working correctly

I believe so:
image

you should be able to just try rerun yarn app-workflow sync and everything just work this time round (if not let me know).

Working now, thanks!

@chrismclarke
Copy link
Member Author

@jfmcquade
I'm going to go ahead and merge here as functional tests appear to be passing and PR blocking a few others.
If you do get a chance to take a look through post-merge then would still be good to hear any questions/comments you might have. I'll also move outstanding follow-up tasks to draft PR #2061

@chrismclarke chrismclarke merged commit 05d1f69 into master Aug 25, 2023
@chrismclarke chrismclarke deleted the refactor/scripts-standalone branch August 25, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants