Improved typechecking error for unstable props

[lucacasonato]

Ref #5175

cc @kitsonk is there a better way of transforming diagnostics? This feels kinda wrong.

[kitsonk]

Not really. I mean there maybe ways to hack the compiler itself.

One thought. I was thinking of having a mirror of the unstable APIs to use in unstable mode, where each would simply be of type never. It would also give an opportunity to put in JSDoc that says "use unstable" for folks in editors, and it should only throw when you try to use it, so less collateral damage. It might be easier to match as well to change the message, though it might be easy enough anyways just to trap the nevers.

Another thing is we could set up a type alias of type unstable = never.

[bartlomieju]

@kitsonk @lucacasonato we should prioritize this issue; there's a lot of questions related to this problem. I like the @kitsonk's idea, but if it requires complicated changes then Luca's solution is also fine.

[lucacasonato]

I'll prototype @kitsonk's idea.

[lucacasonato]

I just tried out some things:

---

// lib.deno.ns.d.ts
export const hostname: never;
// test.ts
const x = Deno.hostname();

which leads to a pretty unhelpful error message:

error: TS2349 [ERROR]: This expression is not callable.
  Type 'never' has no call signatures.
const hello = Deno.hostname();
                   ~~~~~~~~

---

// lib.deno.ns.d.ts
export function hostname(): never;
// test.ts
const x = Deno.hostname();

which leads to no TypeScript error, instead errors at runtime:

error: Uncaught TypeError: Deno.hostname is not a function
const hello = Deno.hostname();
                   ^

---

This PR:

// lib.deno.ns.d.ts

// test.ts
const x = Deno.hostname();

which leads to no TypeScript error:

error: TS2339 [ERROR]: Property 'hostname' does not exist on type 'typeof Deno'. Did you forget to run with the '--unstable' flag? 
const hello = Deno.hostname();
                   ~~~~~~~~

---

I don't think the never thing will work unless I am doing something wrong. I think never means a function will never return rather than it not being meant to run.

[ferm10n]

I got tripped up at least a few times trying to use an unstable function while learning Deno...

Agreeing with kitsonk, we should try to minimize the collateral damage. We're talking about handling error reporting for unstable functions here... if the solution is a hacky one I don't think it'll really matter in the long run once the functions are stable!

But I've been thinking about this for a while and I'd like to add an alternative (which may be more or less hacky, I'm newish to typescript so sorry if my code smells a bit lol)

I can see here where the flag causes unstable stuff to be added. What if we make the unstable properties throw during runtime? Something along these lines...

if (unstableFlag) { // current stuff when unstable is active
    Object.defineProperties(globalThis, unstableMethods);
    Object.defineProperties(globalThis, unstableProperties);
    Object.assign(denoNs, denoUnstableNs);
} else { // when in stable mode, throw if accessing an unstable key (prop/method)
    /** things that should be disallowed, and the object it's for */
    const disallowedUnstableKeys: [Object, string[]][] = [
        [
            globalThis, [
                ...Object.getOwnPropertyNames(unstableMethods),
                ...Object.getOwnPropertyNames(unstableProperties)
            ]
        ],
        [ denoNs, Object.getOwnPropertyNames(denoUnstableNs) ]
    ];

    for (const [ target, disallowedKeysForTarget ] of disallowedUnstableKeys) { // trying to avoid copy-pasting the same thing twice for different targets (globalThis and denoNs)
        Object.defineProperties(
            target,
            disallowedKeysForTarget.reduce((obj, key) => { // for every unstable key, add a getter that will throw
                return {
                    ...obj,
                    [key]: {
                        get () { throw new Error(`Must use the --unstable flag to use '${key}'`); }
                    }
                };
            }, {})
        );
    }
}

Adding this would probably mean that lib.deno.d.ts and lib.deno.unstable.d.ts would need to be merged so the compile can happen.

P.S.: I've been trying/struggling to follow along with Deno's development, but is there a roadmap or something to help us identify what are the most important things to be working on?

[kitsonk]

@lucacasonato I did think that we would want to rewrite TS2349 so it made more sense, but I can see the problem in that you would still want to know about what is a legitimate one and what is one due to --unstable and we are basically back at the original problem.

I think this PRs solution is acceptable, though it might be challenging to maintain. It certainly is worth trying and potentially iterating on if it still causes chaos.

[bartlomieju]

@ferm10n problem with runtime exception is that user can catch and ignore it - and that's not desirable solution - unstable API errors should be terminal.

[ferm10n]

@bartlomieju makes sense, but I would imagine that if a user is going to catch such an error, then they must have seen the error message "must use the --unstable flag" first.

In any case, as soon as the user sees something
that isn't behaving correctly, I would expect they'd start by not ignoring whatever they're catching.

I'm not sure it makes much sense for us to go to such lengths to protect the user from themselves if they are THAT determined to use things incorrectly.

Even still, if we keep the compile time check, a determined enough user could still bypass it and try to use an unstable API without a terminal error if they access it like Deno['utime']... So it feels like kind of a moot point, but that's just me. 🤷‍♂️

[bartlomieju]

@lucacasonato please merge with master.

This PR might be a hack, but it's a huge net improvement to current situation with multiple users being tripped by unhelpful diagnostics.

[bartlomieju]

LGTM, thanks @lucacasonato