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

Type errors when executing denon --version (deno 1.2.0) #74

Closed
Alex0007 opened this issue Jul 13, 2020 · 13 comments
Closed

Type errors when executing denon --version (deno 1.2.0) #74

Alex0007 opened this issue Jul 13, 2020 · 13 comments
Labels
bug Something isn't working deno std Problem introduced with recent updates to the deno std

Comments

@Alex0007
Copy link

❯ denon --version
Check https://deno.land/x/[email protected]/denon.ts
error: TS2345 [ERROR]: Argument of type 'string | URL' is not assignable to parameter of type 'string'.
  Type 'URL' is not assignable to type 'string'.
  return new URL(url).pathname
                 ~~~
    at https://deno.land/[email protected]/path/win32.ts:917:18

TS2345 [ERROR]: Argument of type 'string | URL' is not assignable to parameter of type 'string'.
  Type 'URL' is not assignable to type 'string'.
  return new URL(url).pathname;
                 ~~~
    at https://deno.land/[email protected]/path/posix.ts:438:18

TS2345 [ERROR]: Argument of type 'string | URL' is not assignable to parameter of type 'string'.
  Type 'URL' is not assignable to type 'string'.
  return new URL(url).pathname
                 ~~~
    at https://deno.land/[email protected]/path/win32.ts:911:18

TS2345 [ERROR]: Argument of type 'string | URL' is not assignable to parameter of type 'string'.
  Type 'URL' is not assignable to type 'string'.
  return new URL(url).pathname;
                 ~~~
    at https://deno.land/[email protected]/path/posix.ts:433:18

Found 4 errors.

Setup

  • OS: macOs
  • Deno version: (deno version)
deno 1.2.0
v8 8.5.216
typescript 3.9.2
  • Denon version: (denon --version)
    Latest (i guess)
@Alex0007 Alex0007 added the bug Something isn't working label Jul 13, 2020
@eliassjogreen
Copy link
Member

omelette#43 will solve this once it gets merged.

@koashima
Copy link

I have the same issue. I'm just a beginner and this bug confused me so much. @eliassjogreen hopefully I can try installing once that pr is merged.

@notfilippo notfilippo added the deno std Problem introduced with recent updates to the deno std label Jul 14, 2020
@emmalemma
Copy link

Quick-fix workaround if this just broke on deno upgrade, as it did for me, and you are also on Windows:

  1. Edit the script at ~/.deno/bin/denon.cmd.
  2. Change deno.exe to deno.old.exe.
  3. That's it. It should work until the next time you upgrade deno or denon, by which point hopefully the upstream will be fixed.

If you're not on Windows you might not have deno.old.exe; you'll have to manually download the prior Deno release. Make sure not to overwrite deno, just insert the version you download into the denon script.

This works basically because of Deno's single-exe philosophy; denon still calls the current deno version to run my script even though it's running under a different exe. Color me impressed!

~

I still think this is a serious bug-- it's virtually guaranteed to temporarily break workflows at some point in the future when deno updates.

The root of the issue is that by default denon and my project share a deno version, so updating one is updating the other. In Deno-land, I probably don't mean to do that... it would encourage devs like me to use nodemon instead, just so my workflow has separate dependencies and doesn't break at the same moment my project updates. Obviously that's not ideal for Deno!

This will be properly fixable when something like denoland/deno#986 lands, so denon can be bundled as a self-contained executable, and only update when explicitly recompiled.

Until then, I wonder if it would make sense for denon to support pinning a deno executable to run from? Essentially just preemptively implement my workaround at install time, so devs aren't caught by surprise by some future Deno upgrade. The pin would update when denon updates, so it wouldn't get terribly out of date, but otherwise we assume if it's working, it's working. It could be optional or opt-out for the security conscious.

If that sounds like a good idea to maintainers, I'd be happy to take a look at it at some point-- I'd like to support the ecosystem :)

@brandonkal
Copy link

brandonkal commented Jul 14, 2020

@eliassjogreen That PR has been open for a while. Given that this breaks everything, why are you waiting for a merge? Just point the deps.ts file to resolve omlette as your fork until the change is merged in.

Even when it is merged, it likely won't fix things for many users as that dep is not pinned to a specific version.

notfilippo added a commit that referenced this issue Jul 14, 2020
fix: temp fix for outdated dependencies in omelette (#74)
@notfilippo
Copy link
Collaborator

This should be temporarily resolved. Not closing rn because of @emmalemma interesting suggestion, that definitely needs discussion.

@brandonkal
Copy link

This sort of thing is bound to happen until std becomes stable. Though I disagree with the bundling approach given how heavy deno is now ~60 MB. Using a deno version manager (an example being northscaler/d) would be the approach I'd recommend. In effect, you would replace deno install ... denon.ts with an alternative approach that uses a version manager to ensure the proper deno binary launches the program.

@notfilippo
Copy link
Collaborator

We could hook to d directly, if it's available on the system, and execute code on a specific version with a configuration field / flag. We will definitely look into it

@eliassjogreen
Copy link
Member

A problem I see with d is that it only works on unix. But the idea is good

@ruralant
Copy link

ruralant commented Jul 14, 2020

I experience the same issue with the v2.2.1 version on Mac.
denon --version works fine but as soon as I try to run denon start, I get the following error:

[denon] v2.2.1
[denon] watching path(s): *.*
[denon] watching extensions: ts,tsx,js,jsx,json
[denon] starting `deno run --allow-env --allow-net server.ts`
 S: starting process with pid 49565
 M: monitoring status of process with pid 49565
Check file:///Users/antoniorossi/Development/personal-projects/deno/server.ts
error: TS2345 [ERROR]: Argument of type 'string | URL' is not assignable to parameter of type 'string'.
  Type 'URL' is not assignable to type 'string'.
  return new URL(url).pathname
                 ~~~
    at https://deno.land/[email protected]/path/win32.ts:917:18

TS2345 [ERROR]: Argument of type 'string | URL' is not assignable to parameter of type 'string'.
  Type 'URL' is not assignable to type 'string'.
  return new URL(url).pathname;
                 ~~~
    at https://deno.land/[email protected]/path/posix.ts:438:18

Found 2 errors.
 M: got status of process with pid 49565
 M: process with pid 49565 exited on its own
[denon] app crashed - waiting for file changes before starting ...

@eliassjogreen
Copy link
Member

Did you upgrade using denon --upgrade?

@ruralant
Copy link

Did you upgrade using denon --upgrade?

I didn't. I just tried and it worked. It was the first time I tried denon, I wasn't aware of the --upgrade flag.
Thank you

@emmalemma
Copy link

Good stuff 👍 For the record this is not really an issue with std stability -- imports should be tagged anyway -- but with the deno version, which can't be tagged. Even if std becomes perfectly forward-compatible, someday deno will upgrade to be incompatible with some dependency, and on that day your tooling will break.

Just my two cents, this isn't specific to denon. This will bite any installed script someday, and it'll cost more dev hours every time.

Personally, I'd hope to see a fix in denoland/deno eventually. Deno already does 90% of the work of a version manager: deno upgrade --version= already exists, and deno install already exists. If deno upgrade --named-version=1.1.3 downloaded an exe to deno-1.1.3, and deno install --deno-version=1.1.3 created a script that called it, that would handle this for installed scripts.

There are ecosystem issues to consider, but this is already an ecosystem issue... I wouldn't be surprised to see teams trial and reject Deno over it. It seems worth thinking about a simple, general solution.

@eliassjogreen
Copy link
Member

Closing this as it is not an issue specific to denon and we try to keep denon up to date with all new released deno versions. We have also removed the omelette dep and autocompletion until we implement it again (#100).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deno std Problem introduced with recent updates to the deno std
Projects
None yet
Development

No branches or pull requests

7 participants