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

Better usability when type checking #53

Closed
ry opened this issue May 11, 2021 · 6 comments
Closed

Better usability when type checking #53

ry opened this issue May 11, 2021 · 6 comments
Assignees

Comments

@ry
Copy link
Member

ry commented May 11, 2021

import { h, jsx, serve} from "https://deno.land/x/sift@0.3.0/mod.ts";

const App = () => (
  <div>
    <h1>Hello Work!</h1>
  </div>
);

const NotFound = () => (
  <div>
    <h1>Page not found</h1>
  </div>
);

serve({
  "/": () => jsx(<App />),
  404: () => jsx(<NotFound />, { status: 404 }),
});
# deployctl run index.tsx
Check file:///Users/ry/src/deploy-website/$deno$eval.ts
error: TS6200 [ERROR]: Definitions of the following identifiers conflict with those in another file: DOMException, Event, EventTarget, EventListenerOrEventListenerObject, ProgressEvent, TextDecoder, TextEncoder, AbortController, URLSearchParams, URL, ReadableStreamReader, CountQueuingStrategy, ByteLengthQueuingStrategy, BlobPart, Blob, File, FormDataEntryValue, FormData, HeadersInit, Headers, RequestInfo, RequestCache, RequestCredentials, RequestMode, RequestRedirect, ReferrerPolicy, BodyInit, RequestDestination, Request, ResponseType, Response, CloseEvent, WebSocket, BinaryType, CompileError, Global, Instance, LinkError, Memory, Module, RuntimeError, Table, ImportExportKind, TableKind, ValueType, ExportValue, Exports, ImportValue, ModuleImports, Imports, BufferSource, MessageEvent, ErrorEvent, CustomEvent, Window
interface Account {
~~~~~~~~~
    at asset:///lib.dom.d.ts:25:1

    Conflicts are in this file.
    declare class DOMException extends Error {
    ~~~~~~~
        at file:///Users/ry/Library/Caches/deno/https/deno.land/a54873ce745491942abde8874b5ea92f343a9be4a5413e78dfe7c94fcfe2b201.ts:11:1

however works with

 deployctl run --libs=ns,fetchevent  index.tsx
@kitsonk kitsonk changed the title Type errors in this example Better usability when type checking May 26, 2021
@kitsonk
Copy link

kitsonk commented May 26, 2021

Ok, I have been doing some looking into this and research, a few comments:

  • We have suggested that type checking for the Deno CLI be off by default, which we might do in the future. I believe we should not type check by default for deployctl and have it be opt in.
  • Swapping out lib/types when type checking are unavoidable. There is third party code that drags in its own globals, and that is unavoidable (the example above drags in the TypeScript lib "dom" which is what causes the type errors), but clearly the usability is a big problem. Things we should consider:
    • Using flags like --noLib to disable the window lib, --noFetchEvent to disable the fetchevent lib.

    • I think ns is special, as I think isomoprhic Deno CLI/Deploy code should be our default, so by default I don't think we should include the ns lib, but instead include the deno.ns lib from the CLI, but then have a --noCli flag which would reverse that and only include the deploy.ns lib.

    • That means, by default, isomorphic Deno CLI/Deploy code should "just work", but then code like the above which is designed to be more browser isomorphic as well would become:

      deployctl run --check --noLib index.tsx
      

      And the following would "just run", potentially experiencing runtime errors:

      deployctl run index.tsx
      
    • We can further improve the experience by doing analysis on the diagnostics returned and suggesting that --noLib, --noFetchEvent, --noCli be used.

    • You can't supply arbitrary libs in the --lib flag currently, and I suggest we avoid adding that, so we would just eliminate the --lib argument, as it is confusing. If someone does want to have Deploy code that includes a built in lib, like deno.unstable then they can use the /// <reference lib="deno.unstable" /> directive in their file. We can add documentation to help with those advanced use cases.

  • I think it is better/easier if we don't actually use the "lib" format for the types. I think we should consider UMD type modules which have a declare global { to augment the global namespace in TypeScript.
  • We might want to consider the /// <reference path="..." /> for including the additional types when "injecting" the types when doing the checking, import type {} from "..." works, but is awkward.

@kitsonk
Copy link

kitsonk commented May 27, 2021

Another point I forgot:

  • We can type check properly without having to lazily write a tsconfig.json, we just need to use more triple slash directives in the "wrapping" module:

    /// <reference no-default-lib="true"/>
    /// <reference lib="esnext" />
    /// <reference path="./types/deploy.ns.d.ts" />
    /// <reference path="./types/deploy.fetchevent.d.ts" />
    /// <reference path="./types/deploy.window.d.ts" />

@dsherret
Copy link
Member

dsherret commented Jul 1, 2021

I think whatever solution we go with should type check with the language server and work in Deploy without needing a tsconfig.json or other setup configuration.

I've only been looking at this today and a bit yesterday so I'm probably missing a lot and more than likely don't understand something, but here are my initial thoughts (apologies for over explaining, but it's just to get verification I understand what's going on).

JSX - Type Checking and CLI & Deploy Differences

As discussed, JSX right now in Deploy and the CLI are different because Deploy uses the jsx factory h and and jsx fragement factory Fragment by default, while CLI uses React.createElement and React.Fragment.

This means users who are using the language server will not get a compile error when they use react even though what they're doing would not work on Deploy (they should be forced to specify @jsx React.createElement and @jsxFrag React.Fragment).

Additionally, in order for people to get Deploy JSX to type check happily in the language server and with deno run, they must do the following:

/** @jsx h */
/** @jsxFrag Fragment */
import { h, Fragment } from "https://deno.land/x/sift@0.3.0/mod.ts";

Or add a tsconfig.json that specifies these options.

This would be resolved by the CLI breaking to use h and Fragment instead as at least these are not react specific: denoland/deno#11186

My personal opinion is that I don't like how JSX requires functions to be imported and then magically calls them. I wish we could work towards JSX being transformed to a standalone object (ex. of type Deno.JsxElement) whose structure is then used by libraries. This would eliminate the idea of JSX and fragment factory functions completely, eliminate these magical function calls, and we wouldn't have to transform JSX differently based on the library a user is using. This would be disruptive unfortunately and I don't know enough about JSX to know if it would actually work in practice (I don't see why it wouldn't though).

Type Errors

Ideally the code should type check for the language server, deno run mod.ts, and deploy as-is. I'm not sure if this is possible, but at least perhaps FetchEvent with addEventListener("fetch", ...) could just work. Ryan has mentioned it might make sense to add this to the main worker denoland/deno#5957 (comment)

I don't believe we should reply on a wrapper in deployctl as that does not play well with the code type checking in the language server. Instead, we may want to consider figuring out what triple slash directives might make the most sense and recommend people add that to their code.

deno run mod.ts

It doesn't seem like we should recommend this to users unless:

  1. The fetch event is added to the main worker as mentioned earlier (asking the user to write extra code that registers a service worker specifically for local development doesn't seem ideal)
  2. There is a foolproof way of throwing runtime errors when using unsupported functionality in the Deno object. I'm not sure if such a way exists solely in the CLI or what we could do.

Both these issues seem more easily achievable and less error-prone by telling users to run their Deploy code through a tool like deployctl.

dom types and Deno deploy

The current recommended way to use a library that relies on dom lib types (ex. preact) is to run deployctl with deployctl run --libs=ns,fetchevent script.ts. This prevents deployctl from injecting its deploy.window.d.ts file so that it doesn't collide with differing types in TypeScript's dom.d.ts lib file.

This seems less than ideal as deploy.window.d.ts's types are no longer available or checked against. I'm really not sure what the solution is here because a file having /// <reference lib="dom" /> in it (like preact), is equivalent to compiling with the --lib dom compiler option—it's a global change. Since TypeScript doesn't seem to support module level changes like this (or does it?) and one module using a lib reference directive throws a wrench in things working, I suspect whatever solution we go with here will have to strike some balance between type checking and what actually works at runtime. Perhaps deployctl should just always ensure its types are compatible with dom.d.ts even though a lot of code wouldn't work at runtime. I'm not really sure and was struggling to think of a solution this afternoon.

Edit: it would be nice if TS moved DOM types out https://twitter.com/sanders_n/status/1407030997686702081

You can't supply arbitrary libs in the --lib flag currently, and I suggest we avoid adding that, so we would just eliminate the --lib argument, as it is confusing.

I agree.

@kitsonk
Copy link

kitsonk commented Jul 1, 2021

My personal opinion is that I don't like how JSX requires functions to be imported and then magically calls them. I wish we could work towards JSX being transformed to a standalone object (ex. of type Deno.JsxElement) whose structure is then used by libraries. This would eliminate the idea of JSX and fragment factory functions completely, eliminate these magical function calls, and we wouldn't have to transform JSX differently based on the library a user is using. This would be disruptive unfortunately and I don't know enough about JSX to know if it would actually work in practice (I don't see why it wouldn't though).

I think this is misguided. All this would do is fracture further an already fractured ecosystem and further make Deno incompatible with other ecosystems.

What you don't like is JSX. Transpiling to something other than h() is the least incompatible way. Hyperscript (h()) is the defacto standard for JSX. React.createElement() is the h() API just with a long and potentially global name. When TypeScript is checking it is simply making sure the function that it is transpiling to is in scope. One "solution" could be to make a "global" h() as a default for Deploy which could support rendering, but that is a lot better than some sort of bespoke, custom transpile to Deno.JsxElement which someone would have to somehow re-implement to get back to h().

@dsherret
Copy link
Member

dsherret commented Jul 1, 2021

What you don't like is JSX.

You're not wrong 😅 and yeah I agree that reducing the magic of JSX would be too disruptive and agree that it wouldn't be good for the end user experience given the current state of the system. I guess JSX is just what JSX is at this point.

One "solution" could be to make a "global" h() as a default for Deploy which could support rendering, but that is a lot better than some sort of bespoke, custom transpile to Deno.JsxElement which someone would have to somehow re-implement to get back to h().

Hmmm... yeah I prefer people importing it so we don't risk breaking something for them in the runtime.

Anyway, yeah denoland/deno#11186 looks like a good way forward.

@lucacasonato
Copy link
Member

Thanks for contributing by opening an issue.

This project is being archived because you can now use deno run to run your Deploy scripts locally: https://deno.com/deploy/docs/running-scripts-locally/. As such, I will close this issue.

If you have feedback about Deno Deploy, please open an issue on this repo: https://github.com/denoland/deploy_feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants