feat(cli): support React 17 JSX transforms #12631

kitsonk · 2021-11-03T00:12:43Z

lucacasonato

If I read this correctly, we have to do up to 5 remote fetches now before we know what file to use for the jsx import source? This is quite magical - not really our style. Also no web precedent for poking around in remote servers like this. Also introduces a waterfall behaviour that can significantly slow down use-cases like JIT-at-the-edge transpilation.

cli/emit.rs

kitsonk · 2021-11-03T10:00:21Z

If I read this correctly, we have to do up to 5 remote fetches now before we know what file to use for the jsx import source?

I don't like it either, but if you have some other way of determining it, I am open to suggestions. It is only for something that appears to be the jsx-runtime (or jsx-dev-runtime), and there are various forms already in the wild that need to be supported. React chose an ambiguous module specifier, dependent upon Node resolution rules.

Some examples of "real" specifiers:

https://deno.land/x/[email protected]/jsx-runtime/index.ts
https://cdn.skypack.dev/react/jsx-runtime
https://esm.sh/preact/jsx-runtime

We could say that we would only support situations where the web server resolves/redirects like Skypack and esm.sh do, but that would mean that nano_jsx would not work. Also, local JSX libraries are unlikely to work, as people won't have an extensionless file. We could then maybe say we only do resolution with local specifiers, and expect remote specifiers to be redirects, but again, it makes it impossible then for people to publish things to deno.land/x/ that would work.

Is there an alternative approach I am missing?

lucacasonato · 2021-11-03T10:06:43Z

Is there an alternative approach I am missing?

There are two things I can think of:

we deviate from TSC's resolution logic, and require that users specify a full path. This could possibly be upstreamed to TSC.
we keep TSC's default logic of always just appending /jsx-runtime, and have users use an import map when they want to use a local file. This is sorta what also happens in node nowadays, just not using an import map, but using exports in package.json.

I agree both of these are not great either, but at least they are very clear to what is happening. I think we should avoid probing the FS or remote server at all costs.

kitsonk · 2021-11-03T10:14:06Z

we deviate from TSC's resolution logic, and require that users specify a full path. This could possibly be upstreamed to TSC.

We can't... both swc and tsc emit a determined specifier that cannot be controlled. I have tried to amend it before... it is effectively hard-coded.

For example a jsxImportSource of react emits react/jsx-runtime (or react/jsx-dev-runtime if it is dev mode). It can't be "upstreamed" because it is the behaviour that React expects and Babel and TypeScript have complied with (as well as other transpilers).

we keep TSC's default logic of always just appending /jsx-runtime, and have users use an import map when they want to use a local file. This is sorta what also happens in node nowadays, just not using an import map, but using exports in package.json.

That is the least worst option I guess, though this would not work for Deploy for something like nano_jsx.

lucacasonato · 2021-11-03T10:15:35Z

That is the least worst option I guess, though this would not work for Deploy for something like nano_jsx.

I wouldn't block ourselves on this. Deploy will support import maps eventually.

kitsonk · 2021-11-03T10:19:50Z

That is the least worst option I guess, though this would not work for Deploy for something like nano_jsx.

I wouldn't block ourselves on this. Deploy will support import maps eventually.

Ok, I originally hand't realised that both skypack and esm.sh effectively resolved those for JSX libraries, which I think would cover 90% of the use cases, and then have an example for nano_jsx using an import map would be sufficient I think.

test_util/src/lib.rs

cli/http_util.rs

kitsonk · 2021-11-04T01:04:38Z

To do still:

integrate into the lsp
ensure Deno.emit() works

kitsonk · 2021-11-05T06:28:49Z

There is an outstanding issue on swc (swc-project/swc#2656) that I discovered. It means that:

when someone uses "react-jsxdev" compiler option
and they are using --no-emit or bundling
Deno will error with something like "cannot load "jsx-runtime"`

The workaround is not to use --no-emit or not to use "react-jsxdev". Once fixed it should be transparent and just require updating/uncommenting a few test cases. I am not sure if we would consider it a blocker or not for this feature, but I do think the PR is in a state to be reviewed.

dsherret

LGTM, but I have a few questions.

cli/dts/lib.deno.unstable.d.ts

cli/ast/mod.rs

cli/config_file.rs

cli/tools/test.rs

cli/proc_state.rs

cli/resolver.rs

lucacasonato · 2021-11-08T00:03:26Z

I have tried this out and have found a few little things:

The following case does not work at all with TSC (the emit is broken), but works with SWC:

/** @jsxImportSource https://esm.sh/preact */

function App() {
  return (
    // @ts-ignore
    <div>
      {/* @ts-ignore */}
      <h1>Hello, world!</h1>
      {/* @ts-ignore */}
    </div>
  );
}

<App />;
console.log("Hey!");

$ deno run check-no-import.tsx 
Check file:https:///mnt/starship/Projects/local/testnewjsx/check-no-import.tsx
error: Uncaught ReferenceError: _jsx is not defined
<App />;
^
    at file:https:///mnt/starship/Projects/local/testnewjsx/check-no-import.tsx:14:1
$ deno run --no-check no-check-no-import.tsx
Hey!
$

Secondly, importing from skypack.dev in @jsxImportSource is not really possible, because you always need to add ?dts to the end of the URL. That can be resolved with an import map though, so no big deal. I will bring this up with the skypack folks - maybe ?dts can just become the default.

/** @jsxImportSource https://esm.sh/preact */
/** @jsx h */
/** @jsxFragment Fragment */

The above code works fine (it uses the @jsxImportSource) as expected. It is not obvious to users what should happen here though, so we should add a deno lint to disallow this. One or the other, not both.

We need to add the jsxImportSource option to the config file schema in the VS Code extension.

The following does not work. It is not obvious to me why not. I think it should be working.

import { renderToString } from "https://esm.sh/preact-render-to-string";

function App() {
  return (
    <div>
      <h1>Hello, world!</h1>
    </div>
  );
}

console.log(renderToString(<App />));

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "https://esm.sh/preact",
    "lib": ["esnext", "dom"]
  }
}

$ deno run --config deno.json check.tsx
Check file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx
error: TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
    <div>
    ~~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:7:5

TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
      <h1>Hello, world!</h1>
      ~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:8:7

TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
      <h1>Hello, world!</h1>
                       ~~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:8:24

TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
    </div>
    ~~~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:9:5

Found 4 errors.

lucacasonato · 2021-11-08T00:09:53Z

The following breaks the LSP (the preact in the comment says Resolved Dependency\nCode: [errored] & does not work with deno run --import-map import_map.json check.tsx.

/** @jsxImportSource preact */

import { renderToString } from "https://esm.sh/preact-render-to-string";

function App() {
  return (
    <div>
      <h1>Hello, world!</h1>
    </div>
  );
}

console.log(renderToString(<App />));

{
  "imports": {
    "preact": "https://esm.sh/preact"
  }
}

Changing the import map to this works:

{
  "imports": {
    "preact/jsx-runtime": "https://esm.sh/preact/jsx-runtime"
  }
}

lucacasonato · 2021-11-08T00:11:43Z

And found a little LSP bug: the preact in // @jsxImportSource preact is highlighted, this pragma is only supported in a multi line comment (/** @jsxImportSource preact */).

kitsonk · 2021-11-08T00:41:55Z

The "bad" emit is a TypeScript issue, opened microsoft/TypeScript#46723.

kitsonk · 2021-11-08T01:04:42Z

Secondly, importing from skypack.dev in @jsxImportSource is not really possible, because you always need to add ?dts to the end of the URL. That can be resolved with an import map though, so no big deal. I will bring this up with the skypack folks - maybe ?dts can just become the default.

Yes, I think we have no choice there, as both swc and tsc simply append /jsx-runtime to whatever specifier that is there and we have no control over that.

kitsonk · 2021-11-08T02:59:18Z

The following does not work. It is not obvious to me why not. I think it should be working.

import { renderToString } from "https://esm.sh/preact-render-to-string";

function App() {
  return (
    <div>
      <h1>Hello, world!</h1>
    </div>
  );
}

console.log(renderToString(<App />));

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "https://esm.sh/preact",
    "lib": ["esnext", "dom"]
  }
}

$ deno run --config deno.json check.tsx
Check file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx
error: TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
    <div>
    ~~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:7:5

TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
      <h1>Hello, world!</h1>
      ~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:8:7

TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
      <h1>Hello, world!</h1>
                       ~~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:8:24

TS7026 [ERROR]: JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.
    </div>
    ~~~~~~
    at file:https:///mnt/starship/Projects/local/testnewjsx/check.tsx:9:5

Found 4 errors.

Ok, this is because when importing the https://esm.sh/preact/jsx-runtime from the "config file", we aren't importing the types (X-TypeScript-Type), but when we are importing via @jsxImportSource is "just works". I think it is because when we send it over to tsc, we aren't using the maybe_types_dependency specifier, like we would with other "imports" into a module. Hrmmmfp.

kitsonk · 2021-11-08T04:51:53Z

Ok, this is because when importing the https://esm.sh/preact/jsx-runtime from the "config file", we aren't importing the types (X-TypeScript-Type), but when we are importing via @jsxImportSource is "just works". I think it is because when we send it over to tsc, we aren't using the maybe_types_dependency specifier, like we would with other "imports" into a module. Hrmmmfp.

Ooops. I was wrong... the problem is that resolution doesn't find it, because tsc tries to resolve the runtime module from each .jsx module, realising that it might be different depending on the referrer, and we think it is missing, because it is a "global" dependency, not a specific dependency. Hmmmm...

kitsonk · 2021-11-08T05:57:08Z

Ok, I added denoland/deno_graph#67 which allows us to "find" the jsxImportSource in the module graph. Just need to change the op_resolve in the lsp and cli to look in the right places. It is a blocker for this, as type checking won't work in a lot of real world use cases with just using compiler options instead of the JSX pragma.

kitsonk · 2021-11-08T20:12:41Z

{
  "imports": {
    "preact": "https://esm.sh/preact"
  }
}

@lucacasonato Shouldn't this be:

{
  "imports": {
    "preact/": "https://esm.sh/preact/"
  }
}

lucacasonato · 2021-11-08T21:07:48Z

{
  "imports": {
    "preact": "https://esm.sh/preact"
  }
}

@lucacasonato Shouldn't this be:

{
  "imports": {
    "preact/": "https://esm.sh/preact/"
  }
}

Oh right... my bad... sorry!

kitsonk · 2021-11-09T00:40:36Z

Ok, I have fixed everything that is addressable at the moment, and I think it is good enough to land, but there are two caveats and I will reflect them in the manual:

TSX/JSX modules with no import/exports but using JSX Import Source do not emit properly and will runtime error. Work around is to add export {} to the module or to use --no-check. Real world JSX/TSX modules without any imports or exports are likely rare in the wild.
--no-check/bundling (using swc as the emitter) does not properly support the compiler option "react-jsxdev". The work-around is to use "react-jsx" or to not use --no-check or bundling. Using "react-jsxdev" has limited value in Deno anyways.

Both of these are caused by upstream issues, which are open and being tracked.

lucacasonato

Let's ship it!

CanRau · 2021-11-09T04:59:19Z

Using "react-jsxdev" has limited value in Deno anyways.

Curious why that has limited value?

kitsonk · 2021-11-09T06:55:25Z

Using "react-jsxdev" has limited value in Deno anyways.

Curious why that has limited value?

Because most JSX runtimes, except React, don't actually use it, and in most cases, people use JSX for SSR, and the instrumentation for the dev mode is focused on client side debugging, with all the useful information being erased when SSR occurs.

CanRau · 2021-11-13T05:50:35Z

Using "react-jsxdev" has limited value in Deno anyways.

Curious why that has limited value?

Because most JSX runtimes, except React, don't actually use it, and in most cases, people use JSX for SSR, and the instrumentation for the dev mode is focused on client side debugging, with all the useful information being erased when SSR occurs.

Uuh I see, makes sense, thanks for elaborating 🙏

pheuter · 2022-01-16T20:31:08Z

I'm seeing an issue with this when trying to do deno bundle with react/jsx-runtime, create repro here with description of error: https://github.com/pheuter/hello-deno#blocking-issue.

Currently unable to bundle client-side code using react/jsx-runtime despite deno run working just fine using the same code and config:

» deno bundle --config deno.json client/index.tsx static/client.js
Check file:https:///Users/mark/src/pheuter/hello-deno/client/index.tsx
Bundle file:https:///Users/mark/src/pheuter/hello-deno/client/index.tsx
error: Unable to output during bundling.

Caused by:
    0: load_transformed failed
    1: failed to analyze module
    2: failed to resolve https://esm.sh/[email protected]/jsx-runtime from file:https:///Users/mark/src/pheuter/hello-deno/client/index.tsx
    3: Cannot resolve "https://esm.sh/[email protected]/jsx-runtime" from "file:https:///Users/mark/src/pheuter/hello-deno/client/index.tsx".

» deno --version
deno 1.17.3 (release, aarch64-apple-darwin)
v8 9.7.106.15
typescript 4.5.2

Oddly enough, the issue appears to go away when I explicitly do /** @jsxImportSource https://esm.sh/[email protected] */ in every tsx file, which I'm trying to avoid with this optimization in the first place.

kitsonk · 2022-01-16T20:42:51Z

Please open an issue and not comment on a PR that has been closed for over 2 months unrelated to the issue you are experiencing.

pheuter · 2022-01-16T20:46:57Z

No problem, thought I saw some potentially relevant comments above, wasn't sure the best way to proceed. Went ahead and opened #13389.

lucacasonato reviewed Nov 3, 2021

View reviewed changes

cli/emit.rs Outdated Show resolved Hide resolved

kitsonk force-pushed the kitsonk/issue8440 branch from d7044d0 to 8c81ea1 Compare November 3, 2021 09:43

kitsonk force-pushed the kitsonk/issue8440 branch from 8c81ea1 to 55ba03f Compare November 3, 2021 23:44

kitsonk commented Nov 3, 2021

View reviewed changes

test_util/src/lib.rs Show resolved Hide resolved

kitsonk commented Nov 3, 2021

View reviewed changes

cli/http_util.rs Show resolved Hide resolved

kitsonk requested review from dsherret and lucacasonato November 5, 2021 06:23

kitsonk marked this pull request as ready for review November 5, 2021 06:23

kitsonk changed the title ~~[WIP] feat(cli): support React 17 JSX transforms~~ feat(cli): support React 17 JSX transforms Nov 5, 2021

kitsonk force-pushed the kitsonk/issue8440 branch from 9250501 to a03105e Compare November 7, 2021 00:15

dsherret approved these changes Nov 7, 2021

View reviewed changes

cli/dts/lib.deno.unstable.d.ts Outdated Show resolved Hide resolved

cli/ast/mod.rs Show resolved Hide resolved

cli/config_file.rs Outdated Show resolved Hide resolved

cli/tools/test.rs Show resolved Hide resolved

cli/proc_state.rs Show resolved Hide resolved

dsherret reviewed Nov 7, 2021

View reviewed changes

cli/resolver.rs Show resolved Hide resolved

lucacasonato added this to the 1.16.0 milestone Nov 8, 2021

This was referenced Nov 8, 2021

@jsxImportSource only valid on multi-line comments denoland/deno_graph#66

Closed

Introduce an aggregate resolver #12684

Closed

kitsonk mentioned this pull request Nov 8, 2021

Lint proper use of jsx pragma denoland/deno_lint#969

Open

kitsonk force-pushed the kitsonk/issue8440 branch from a03105e to 8d872d0 Compare November 8, 2021 04:20

kitsonk added 11 commits November 9, 2021 11:14

feat(cli): support JSX transforms

a9d3039

fixup

24e76de

refactor to not 'sniff' for jsx import sources

490f1d7

fixups

99aee02

add Deno.emit and no_check support

b73ed2f

add tests for Deno.emit()

f143ff3

update Cargo.toml

1fe8a89

add jsxImportSource to config schema

814d991

address most feedback

527e2d7

add support to lsp for resolving config file JSX import source

ea2dda2

fix caching of jsx import source in lsp

9c7a197

kitsonk force-pushed the kitsonk/issue8440 branch from 2b9beb1 to 9c7a197 Compare November 9, 2021 00:33

lucacasonato approved these changes Nov 9, 2021

View reviewed changes

kitsonk merged commit f5eb177 into denoland:main Nov 9, 2021

kitsonk deleted the kitsonk/issue8440 branch November 9, 2021 01:26

kitsonk mentioned this pull request Mar 12, 2022

Determine correct moduleDetection compiler option (TypeScript 4.7) #13924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): support React 17 JSX transforms #12631

feat(cli): support React 17 JSX transforms #12631

kitsonk commented Nov 3, 2021

lucacasonato left a comment •

edited

Loading

kitsonk commented Nov 3, 2021

lucacasonato commented Nov 3, 2021

kitsonk commented Nov 3, 2021

lucacasonato commented Nov 3, 2021

kitsonk commented Nov 3, 2021

kitsonk commented Nov 4, 2021 •

edited

Loading

kitsonk commented Nov 5, 2021

dsherret left a comment

lucacasonato commented Nov 8, 2021 •

edited

Loading

lucacasonato commented Nov 8, 2021

lucacasonato commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

lucacasonato commented Nov 8, 2021

kitsonk commented Nov 9, 2021

lucacasonato left a comment

CanRau commented Nov 9, 2021

kitsonk commented Nov 9, 2021

CanRau commented Nov 13, 2021

pheuter commented Jan 16, 2022

kitsonk commented Jan 16, 2022

pheuter commented Jan 16, 2022

feat(cli): support React 17 JSX transforms #12631

feat(cli): support React 17 JSX transforms #12631

Conversation

kitsonk commented Nov 3, 2021

lucacasonato left a comment • edited Loading

Choose a reason for hiding this comment

kitsonk commented Nov 3, 2021

lucacasonato commented Nov 3, 2021

kitsonk commented Nov 3, 2021

lucacasonato commented Nov 3, 2021

kitsonk commented Nov 3, 2021

kitsonk commented Nov 4, 2021 • edited Loading

kitsonk commented Nov 5, 2021

dsherret left a comment

Choose a reason for hiding this comment

lucacasonato commented Nov 8, 2021 • edited Loading

lucacasonato commented Nov 8, 2021

lucacasonato commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

kitsonk commented Nov 8, 2021

lucacasonato commented Nov 8, 2021

kitsonk commented Nov 9, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

CanRau commented Nov 9, 2021

kitsonk commented Nov 9, 2021

CanRau commented Nov 13, 2021

pheuter commented Jan 16, 2022

kitsonk commented Jan 16, 2022

pheuter commented Jan 16, 2022

lucacasonato left a comment •

edited

Loading

kitsonk commented Nov 4, 2021 •

edited

Loading

lucacasonato commented Nov 8, 2021 •

edited

Loading