Add prompts to ask user about installing additional but required packages #653

dawiidio · 2023-10-28T16:17:03Z

It's just a small change but can improve flow while using cli.

When using devs devtools src/main.ts with "native" OS transports (eg: usb or serial - not via dashboard and WebUSB) user must specify transport by passing one of the following options

-u, --usb             listen to Jacdac over USB (requires usb)
-s, --serial          listen to Jacdac over SERIAL (requires serialport)
-i, --spi             listen to Jacdac over SPI (requires rpio, experimental)

all the above options require additional packages, that's why now you can install required package without leaving current process. Cli will detect if any of native transports is enabled and if required package is not installed will prompt question like the below one:

...
Install package "serialport" (Y/n)?
Installing package "serialport" ...
Package "serialport" installed!
...

after installation normal process flow will be restored with installed dependencies ready to use.

…d packages

dawiidio · 2023-10-28T16:20:44Z

@microsoft-github-policy-service agree

pelikhan · 2023-10-28T16:32:48Z

cli/src/packageInstaller.ts

+    let resolve: () => void
+    let reject: () => void
+
+    const installProcess = spawn('npm', ['install', '--save', '--no-workspaces', packageName], {


I think we have some code to detect users that rely on yarn

sure, detection added in this commit d806288 and small refactor to use the same function in this commit 11cd434

pelikhan · 2023-10-28T16:33:21Z

cli/src/build.ts

@@ -573,7 +591,7 @@ async function buildOnce(file: string, options: BuildOptions) {
            .forEach(fn => {
                log(`  ${fn.name} (${prettySize(fn.size)})`)
                fn.users.forEach(user =>
-                    debug(`    <-- ${resolver.posToString(user[0])}`)
+                    debug(`    <-- ${resolver.posToString(user[0])}`),


is this , added by prettier? it kind of add noise to the PR

yeah, must be prettier. Do you want to remove it?

Yes, in general this kind of formatting changes really add noise to a PR and makes the review harder. If you have user settings overriding the repo .prettierrc file, you should disable them.

pelikhan · 2023-10-28T16:34:29Z

cli/src/transport.ts

-function createSPI() {
-    log(`adding SPI transport (requires "rpio" package)`)
+async function createSPI() {
+    await askForPackageInstallation('rpio');


We need a non-interactive mode in CI build mode.

Good point! Flag added globally to the cli in this commit ddf9c15

pelikhan · 2023-10-28T16:52:25Z

This is a great start! My advice is to keep you PRs very focused on a single feature. If you find a bug along the way, move it out to a different PR so that it can be merged separately.

The formatting issue is annoying (the added ,) maybe we can fix this before this PR so that it is "just a formatting PR".

A few more comments:

when the native package is missing and the cli is in vscode mode, we should send a side channel message to vscode so that the install flow is gone through vscode UI (future PR)
need a flag to disable all interactions behavior for CI scenarios

👍 keep going!

dawiidio · 2023-10-29T13:15:18Z

@pelikhan

when the native package is missing and the cli is in vscode mode, we should send a side channel message to vscode so that the install flow is gone through vscode UI (future PR)

sure, I can add this communication in next pr. Maybe you know, when cli is in vscode mode it opens some additional socket channel to communicate with plugin?

cli/src/build.ts

pelikhan · 2023-10-29T17:13:10Z

@pelikhan

when the native package is missing and the cli is in vscode mode, we should send a side channel message to vscode so that the install flow is gone through vscode UI (future PR)

sure, I can add this communication in next pr. Maybe you know, when cli is in vscode mode it opens some additional socket channel to communicate with plugin?

Search for export function sendEvent<T extends SideEvent>( to see how to send an event from the CLI, and subSideEvent<SideOutputEvent> to receive that even on the vscode side

pelikhan · 2023-10-30T15:57:37Z

I've added a common linting task in package.json and also a dep on prettier so that we run the same version. You should be able to merge origin/main and it'll fix the whitespace/comma changes.

pelikhan · 2023-10-30T15:40:44Z

cli/src/build.ts

-                _ => null
-            )
-            if (prelude[fn] != ex) await writeFile(fnpath, prelude[fn])
+            if (!existsSync(libpath))


don't sneak in unrelated changes.

pelikhan · 2023-10-30T15:41:12Z

cli/src/build.ts

        const converter = converters()[lang]
        let constants = ""
        for (const srv of customServices) {
            constants += converter(srv) + "\n"
        }
        const dir = join(pref, GENDIR, lang)
-        await mkdirp(dir)
-        return writeFile(join(dir, `constants.${lang}`), constants, {
+        if (!existsSync(dir))


unrelated changes, move to differfent PR

pelikhan · 2023-10-30T15:41:23Z

cli/src/build.ts

            encoding: "utf-8",
        })
    }))
    // json specs
    {
        const dir = join(pref, GENDIR)
-        await mkdirp(dir)
+        if (!existsSync(dir))


unrelated changes, move to different PR

pelikhan · 2023-10-30T15:42:08Z

cli/src/devtools.ts

@@ -430,6 +430,8 @@ async function rebuild(args: BuildReqArgs) {
    opts.verify = false
    opts.quiet = true

+    await compileFile(args.filename, opts);


are we compiling twice now?

pelikhan · 2023-10-30T16:15:22Z

Merge origin/main, run yarn install and run yarn lint to run prettier

pelikhan · 2023-11-02T15:37:08Z

@dawiidio i've added a commit (4372a14) to prep for this change. You can squeeze in the interactive piece of your changes in the TODO I left. Might be easier to start a new PR though.

pelikhan · 2023-11-06T15:46:50Z

Thank you @dawiidio , your PR has made it into #656

feat: add prompts to ask user about installing additional but require…

f081962

…d packages

dawiidio mentioned this pull request Oct 28, 2023

No DeviceScript Managers found #651

Closed

chore: remove console log

1619577

pelikhan reviewed Oct 28, 2023

View reviewed changes

dawiidio added 5 commits October 29, 2023 13:26

feat: detect package manager - yarn or npm

d806288

fix: await for transports

a9a3ef2

feat: interactive mode flag

ddf9c15

chore: refactor yarn detection code

11cd434

chore: remove log

bb0e813

pelikhan reviewed Oct 29, 2023

View reviewed changes

cli/src/build.ts Show resolved Hide resolved

pelikhan requested changes Oct 30, 2023

View reviewed changes

Merge branch 'main' into prompts_for_native_transports

1311418

pelikhan self-requested a review October 30, 2023 16:13

pelikhan added a commit that referenced this pull request Nov 6, 2023

integrating #653

d374900

pelikhan mentioned this pull request Nov 6, 2023

Update PR for https://github.com/microsoft/devicescript/pull/653 #656

Merged

pelikhan added a commit that referenced this pull request Nov 6, 2023

Update PR for #653 (#656)

e9b3d1f

pelikhan closed this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prompts to ask user about installing additional but required packages #653

Add prompts to ask user about installing additional but required packages #653

dawiidio commented Oct 28, 2023

dawiidio commented Oct 28, 2023

pelikhan Oct 28, 2023

dawiidio Oct 29, 2023 •

edited

Loading

pelikhan Oct 28, 2023

dawiidio Oct 29, 2023

pelikhan Oct 29, 2023

pelikhan Oct 28, 2023

dawiidio Oct 29, 2023

pelikhan commented Oct 28, 2023

dawiidio commented Oct 29, 2023

pelikhan commented Oct 29, 2023

pelikhan commented Oct 30, 2023

pelikhan Oct 30, 2023

pelikhan Oct 30, 2023

pelikhan Oct 30, 2023

pelikhan Oct 30, 2023

pelikhan commented Oct 30, 2023

pelikhan commented Nov 2, 2023

pelikhan commented Nov 6, 2023

Add prompts to ask user about installing additional but required packages #653

Add prompts to ask user about installing additional but required packages #653

Conversation

dawiidio commented Oct 28, 2023

dawiidio commented Oct 28, 2023

Choose a reason for hiding this comment

dawiidio Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelikhan commented Oct 28, 2023

dawiidio commented Oct 29, 2023

pelikhan commented Oct 29, 2023

pelikhan commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelikhan commented Oct 30, 2023

pelikhan commented Nov 2, 2023

pelikhan commented Nov 6, 2023

dawiidio Oct 29, 2023 •

edited

Loading