ubports / promise-android-tools Goto Github PK

JSDoc blocks should be added to all our functions.

See ubports/ubports-installer#1008

We use [command] | grep -v something to filter out all lines containing "something" on macOS and Linux, but that obviously does not work on Windows. An alternative could be findstr. Someone has to figure out the correct syntax and implement it here.

Move to ES6 for the awesome of it all. CommonJS who?

Thanks for all the great work on the conversion to typescript - this actually cleared up many things on my side.

Was wondering if we could get a push to the npm repository of the latest, at there seems to be a version/code mismatch.

I can submit a PR for the following changes as well;

strazzere@1939df6

Though essentially I added rollup back as a dev dependency, added it to the build command since the main entry point in the package.json has no was to be created. Then I added the prepare script which allows a user to do a direct npm install https://github.com/theuser/therepo.

The public/private/protected keywords are missing on some methods.

adb push already supports multiple files. These functions could be a lot leaner with a lot less code. It is of course imperative that we keep the option to report the progress at set intervals.

Cowboy coding results in bad practice.

Since CancelablePromise has been deprecated in 5.x, we can now properly use async/await.

CHANGELOG.md also did not get updated for 4.0.9.

To accomplish things like detecting any connected device (eg. ubports/ubports-installer#902), we could reshape module.js to be its own class with instances of all available tools as properties. That way we could create some convenience functions like a tool-agnositc waitForDevice() using something along the lines of Promise.race([adb.waitForDevice(),fastboot.waitForDevice(),heimdall.waitForDevice()]) and returning a string describing the connected mode. You could also go further and attempt os detection and device name detection.

Docs should be available online!

We should at lease cover all relevant lines, ideally even all branches. See coveralls for current progress.

For long-running tasks, child_process.exec() is not suitable, because it can lead to process termination when the stdout buffer is full. We also do not have access to the stdout as it is being generated, making it impossible to make use of things like adb push progress reporting and requiring dirty hacks.

this would make analyzing stuff like https://twitter.com/Toz1k/status/1336672048173752322 easier

We might think about batching execs (across tools?) through dataloader to make sure the device isn't bombarded with too many things at once.

We are not enforcing some compiler options yet, it might be useful in the long run to keep code quality up.

There are some things shared among all tools. Creating a super-class would allow us to get rid of some duplicate (testing-)code and improve maintainability.

This took quiet a while to track down and implement a work around - though I must admit, it feels like there should be another way to do this. Please take this report with a grain of salt as I may be missing the actual underlying issue as it only reproduces in some environments.

While attempting to do a simple adb operation like this following;

 const { Adb } = require("promise-android-tools")
 adb = new Adb({serialno:"12345" })
 await adb.push(['tsconfig.json'], '/data/local/tmp/derp')

The package would spit out a TypeError: Illegal Instruction on the line this.signal.throwIfAborted() inside Tool._withEnv.

This does not reproduce when running solely in a node environment, the this object appears fine. However in the electron environment where it fails, the this object appears to be a "less than deep" clone, or the this context appears to get mangled somehow - this part is a bit unclear to me.

I believe this is based on how Object.create doesn't actually run the constructor. Tested a few other methods for deep cloning the object (Object.assign({}, this), { ... this}, ...) - thought none of them seemed to solve them. Since the type is unpredictable (Adb, FastBoot, Heimdall) it seemed best to try to reflectively call this's own type constructor and just pass this to it. This resulted in the following patch;

diff --git a/src/tool.ts b/src/tool.ts
index c19b017..c1b678b 100644
--- a/src/tool.ts
+++ b/src/tool.ts
@@ -227,7 +227,8 @@ export abstract class Tool extends Interface {
 
   /** returns clone with variation in env vars */
   public _withEnv(env: NodeJS.ProcessEnv): this {
-    const ret = Object.create(this);
+    // @ts-ignore-next-line
+    const ret = new this.constructor(this);
     ret.extraEnv = { ...this.extraEnv };
     for (const key in env) {
       if (Object.hasOwnProperty.call(env, key)) {

This does work for my usecase, and seemingly all other spawn commands I tried, though I honestly hate it due to the ts-ignore.

Hoping to potentially learn something here if anyone knows a better way to fix this issue, or if maybe I'm completely misunderstanding this issue. Whatever the solution, it would likely need to be applied to a few other spots which utilize the Object.create(this) pattern.

If the solution I found above seems the best fitting, I can always draft a PR to fix this and other instance with a bit of a commented explanation.

Cheers!

Granted, we can't get a progress on every file flashed, but since we're flashing sequentially, it'd be fairly straightforward to get a progress callback after every file flashed. When flashing three or more files, the experience would be a lot better!

It was a really stupid idea of mine to put error handling for the different tools into the common module. Split it to separate functions inside the respective modules.

If someone is ever looking for something ✨ fun ✨ to do: The docs comments could use someone going through them and simplifying/standardizing them in a way that improves their usefulness and accessibility.

Someone at Samsung decided it was a good idea to not follow standards, and i hope to god there is a special place in hell for them where they suffer eternal damnation. We can already get our own suffering on by preparing to support Heimdall.