Skip to content

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented May 24, 2024

This PR correctly get prompt result for record to update

This should solve all our breakage with publish and update

For the story, this is a breakage since 1.5.23 from an old PR (#8907) that we missed because we don't CI testing of quarto publish command, nor some Steps to follow for Manual testing to do when we modify these parts of the code base.

Though this one would have been tricky because if needs prompt selection - so only manual testing would have help.

I'll test the other provider before merging I'll do that next week.

This should solve all our breakage with publish and update
@cderv
Copy link
Collaborator Author

cderv commented May 24, 2024

again something is not right between my local testing and CI 😞

Making this a draft for now

@cderv cderv marked this pull request as draft May 24, 2024 16:10
@cderv
Copy link
Collaborator Author

cderv commented May 24, 2024

Check file:///D:/a/quarto-cli/quarto-cli/tests/integration/mermaid/github-issue-1340.test.ts
error: TS2367 [ERROR]: This comparison appears to be unintentional because the types '{ name: string; value: string; }' and 'string' have no overlap.
  if (confirm !== kOther) {
      ~~~~~~~~~~~~~~~~~~
    at file:///D:/a/quarto-cli/quarto-cli/src/command/publish/deployment.ts:235:7

TS2367 [ERROR]: This comparison appears to be unintentional because the types 'string' and '{ name: string; value: string; }' have no overlap.
      publishRecordIdentifier(deployment.target, deployment.account) ===
      ^
    at file:///D:/a/quarto-cli/quarto-cli/src/command/publish/deployment.ts:237:7

Found 2 errors.

Again a type check issue that I don't have locally. And when debugging, confirm is a string ! (here:

const confirm = await Select.prompt({
indent: "",
message: "Publish update to:",
options,
});

So really not sure what is going on. The issue for me (and hence this PR) is that confirm.value is undefined. Hence why the publishing does not update.

I'll keep looking - I am thinking that while doing #8907 the same issue happens, and fix was made to use .value - but when used this is not working as confirm is a string.

@cderv
Copy link
Collaborator Author

cderv commented May 24, 2024

Ok so I can reproduce the error while running test locally

❯ ./run-tests.ps1 integration/mermaid/github-issue-1340.test.ts
> Setting all the paths required...
> Preparing running tests...
> Activating virtualenv for Python tests in Quarto
Courtesy Notice: Pipenv found itself running within a virtual environment, so it will automatically use that environment, instead of creating its own for any project. You can set PIPENV_IGNORE_VIRTUALENVS=1 to force pipenv to ignore that environment and create its own instead. You can set PIPENV_VERBOSITY=-1 to suppress this warning.
> Using Python from C:\Users\chris\Documents\DEV_R\quarto-cli\tests\.venv/Scripts\python.exe
> VIRTUAL_ENV: C:\Users\chris\Documents\DEV_R\quarto-cli\tests\.venv
> Running tests with "C:\Users\chris\Documents\DEV_R\quarto-cli\package\dist\bin\tools\x86_64\deno.exe test --config test-conf.json --unstable-ffi --allow-read --allow-write --allow-run --allow-env --allow-net --check --importmap=C:\Users\chris\Documents\DEV_R\quarto-cli\src\dev_import_map.json integration/mermaid/github-issue-1340.test.ts"
Check file:///C:/Users/chris/Documents/DEV_R/quarto-cli/tests/integration/mermaid/github-issue-1340.test.ts
error: TS2367 [ERROR]: This comparison appears to be unintentional because the types '{ name: string; value: string; }' and 'string' have no overlap.
  if (confirm !== kOther) {
      ~~~~~~~~~~~~~~~~~~
    at file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/command/publish/deployment.ts:235:7

TS2367 [ERROR]: This comparison appears to be unintentional because the types 'string' and '{ name: string; value: string; }' have no overlap.
      publishRecordIdentifier(deployment.target, deployment.account) ===
      ^
    at file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/command/publish/deployment.ts:237:7

Found 2 errors.

Se we definitely have something between test environment and running environment

@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label May 30, 2024
@cderv
Copy link
Collaborator Author

cderv commented May 30, 2024

Ok so after discussing with @gordonwoodhull, I tried to check everything on Ubuntu also; I can see the check issue locally, but still I see a problem with what we do.

here is a minimal reproducible example with Deno

 ./package/dist/bin/tools/deno
Deno 1.41.0
exit using ctrl+d, ctrl+c, or close()
REPL is running with all permissions allowed.
To specify permissions, run `deno repl` with allow flags.
> import { Select } from "https://deno.land/x/cliffy@v1.0.0-rc.3/prompt/select.ts";
undefined
> const options=[{ name: "a", value: "b"}, {name: "c", value: "d"}];
undefined
> const confirm = await Select.prompt({
    indent: "",
    message: "Publish update to:",
    options,
  });
? Publish update to:  c
undefined
> confirm
"d"

This is replicating what we do, and so confirm is a string, not a { name: string, value: string} object, as the type checking error says.

So I am quite puzzled on this...

The result we get seems in line with what I can find about cliffy

but the validation does not say the same... Is this a problem with cliffy ?

@cderv
Copy link
Collaborator Author

cderv commented May 30, 2024

To confirm my understanding I went at the deno level.

if I do this scripts test.ts

import { Select } from "https://deno.land/x/cliffy@v1.0.0-rc.4/prompt/mod.ts";

const options=[{ name: "a", value: "b"}, {name: "c", value: "d"}];

const confirm = await Select.prompt({
    indent: "",
    message: "Publish update to:",
    options,
  });

if (confirm !== "d") {
  console.log("value is not d")
} else {
  console.log("value is d")
}

console.log(confirm)

and now run deno run test.ts it works ok

❯ deno run test.ts
? Publish update to: › a
value is not d
b

But if I check it that it errors

~/project/quarto-cli/package/dist/bin/tools/deno check test.ts
Check file:///tmp/quarto/test.ts
error: TS2367 [ERROR]: This comparison appears to be unintentional because the types '{ name: string; value: string; }' and 'string' have no overlap.
if (confirm !== "d") {
    ~~~~~~~~~~~~~~~
    at file:///tmp/quarto/test.ts:11:5

If I modify to add the confirm.value

import { Select } from "https://deno.land/x/cliffy@v1.0.0-rc.4/prompt/mod.ts";

const options=[{ name: "a", value: "b"}, {name: "c", value: "d"}];

const confirm = await Select.prompt({
    indent: "",
    message: "Publish update to:",
    options,
  });

if (confirm.value !== "d") {
  console.log("value is not d")
} else {
  console.log("value is d")
}

console.log(`confirm.value is ${confirm.value}`)
console.log(`confirm is: ${confirm}`)

it passes the checks but... we get wrong value

❯ deno run test.ts
? Publish update to: › c
value is not d
confirm.value is undefined
confirm is: d

So issue with cliffy and type checking or something I need to learn about how all this works ?

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented May 30, 2024

I think that Typescript is inferring the wrong type for Select.prompt()and the error is spurious.

I think this is because Select.prompt() can take many different types for its options and Typescript picks the wrong one.

https://deno.land/x/cliffy@v1.0.0-rc.1/prompt/select.ts?s=Select

In my fairly-new-to-Typescript opinion, it needs the nudge:

const confirm = await Select.prompt<string>({

In practice the method always returns the value type, but here Typescript is inferring the {name,value} type wrongly.

So the update of deployment.ts in 396a8d7 was incorrect

-  if (confirm !== kOther) {
+  if (confirm.value !== kOther) {
     return depoyments.find((deployment) =>
-      publishRecordIdentifier(deployment.target, deployment.account) === confirm
+      publishRecordIdentifier(deployment.target, deployment.account) ===
+        confirm.value
     );
   } else {
     return undefined;

And we can't 100% trust Typescript errors when doing collateral evolution / API migration on an upgrade.

@cscheid
Copy link
Collaborator

cscheid commented May 30, 2024

And we can't 100% trust Typescript errors when doing collateral evolution / API migration on an upgrade.

Just to add to this, Cliffy does a lot of really fancy type-level work, and has failed similarly in the past from one version upgrade to the next. You'll find a number of any sprinkled in our code base near the Cliffy commands because of things like this.

@gordonwoodhull
Copy link
Contributor

This doesn’t explain why @cderv does not see the error in Windows. I hope that type inference is not platform-dependent!

@cderv
Copy link
Collaborator Author

cderv commented May 31, 2024

Thanks a lot. I'll find a correct syntax that works everywhere, and try to understand why it does not fail on my side; I know what to do now.

Looking through our code, I found other similar usage of Select.prompt that does not trigger a TypeCheck problem, while it is really exactly same usage (i.e. options is {name: string, value: string} and we compare than with results as string)

if (await slugAvailable(slug)) {
const kConfirmed = "confirmed";
const input = await Select.prompt({
indent: "",
message: `${typeName(type)} name:`,
options: [
{
name: slug,
value: kConfirmed,
},
{
name: "Use another name...",
value: "another",
},
],
});
if (input === kConfirmed) {
return slug;
}
}

So it may be related on how the options are passed, and how their type is determined. Because modifying to this

diff --git a/src/publish/common/publish.ts b/src/publish/common/publish.ts
index 2781b7e82..55d0b7deb 100644
--- a/src/publish/common/publish.ts
+++ b/src/publish/common/publish.ts
@@ -346,19 +346,20 @@ async function promptForSlug(
 
   if (await slugAvailable(slug)) {
     const kConfirmed = "confirmed";
+    const opts = [
+      {
+        name: slug,
+        value: kConfirmed,
+      },
+      {
+        name: "Use another name...",
+        value: "another",
+      },
+    ];
     const input = await Select.prompt({
       indent: "",
       message: `${typeName(type)} name:`,
-      options: [
-        {
-          name: slug,
-          value: kConfirmed,
-        },
-        {
-          name: "Use another name...",
-          value: "another",
-        },
-      ],
+      options: opts,
     });
     if (input === kConfirmed) {
       return slug;

gives me the error now
image

Good to know that cliffy can have this specific handling we need to managed !

due to wrong  output type guessing when using object variable as options
@cderv cderv marked this pull request as ready for review May 31, 2024 09:08
@cderv cderv removed the needs-discussion Issues that require a team-wide discussion before proceeding further label May 31, 2024
@cderv cderv merged commit 92bfd31 into main May 31, 2024
@cderv cderv deleted the fix/publish-connect-update branch May 31, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants