-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat(cli): support React 17 JSX transforms #12631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d7044d0
to
8c81ea1
Compare
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 Some examples of "real" specifiers:
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 Is there an alternative approach I am missing? |
There are two things I can think of:
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. |
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
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. |
8c81ea1
to
55ba03f
Compare
To do still:
|
There is an outstanding issue on swc (swc-project/swc#2656) that I discovered. It means that:
The workaround is not to use |
9250501
to
a03105e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I have a few questions.
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 https://esm.sh/preact */
/** @jsx h */
/** @jsxFragment Fragment */ The above code works fine (it uses the We need to add the 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. |
The following breaks the LSP (the /** @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"
}
} |
And found a little LSP bug: the |
The "bad" emit is a TypeScript issue, opened microsoft/TypeScript#46723. |
Yes, I think we have no choice there, as both swc and tsc simply append |
Ok, this is because when importing the |
a03105e
to
8d872d0
Compare
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... |
Ok, I added denoland/deno_graph#67 which allows us to "find" the jsxImportSource in the module graph. Just need to change the |
@lucacasonato Shouldn't this be: {
"imports": {
"preact/": "https://esm.sh/preact/"
}
} |
Oh right... my bad... sorry! |
2b9beb1
to
9c7a197
Compare
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:
Both of these are caused by upstream issues, which are open and being tracked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ship it!
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 🙏 |
I'm seeing an issue with this when trying to do Currently unable to bundle client-side code using
» 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 |
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. |
No problem, thought I saw some potentially relevant comments above, wasn't sure the best way to proceed. Went ahead and opened #13389. |
Closes #8440