Add support for jsx-runtime #129

JacopoPatroclo · 2024-02-18T20:42:51Z

It would be nice to support the auto-import of the jsx runtime, removing the need to import the @kitajs/html/register sub-package.

This PR aims to do that.

@arthurfiorette let me know if this seems like an improvement and if you would like to make this the standard way of configuring the tsconfig for this library. In that case, I will adapt the docs.

codecov · 2024-02-18T20:43:48Z

Codecov Report

Attention: Patch coverage is 82.27848% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 97.41%. Comparing base (5569733) to head (2580c01).

❗ Current head 2580c01 differs from pull request most recent head f4656f4. Consider uploading reports for the commit f4656f4 to get more accurate results

Files	Patch %	Lines
index.js	78.46%	14 Missing ⚠️
jsx-runtime.js	84.26%	14 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##            master     #129      +/-   ##
===========================================
- Coverage   100.00%   97.41%   -2.59%     
===========================================
  Files            4        5       +1     
  Lines          964     1082     +118     
===========================================
+ Hits           964     1054      +90     
- Misses           0       28      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arthurfiorette · 2024-02-18T20:53:07Z

Hey, can you give more details about what and why you're doing this?

JacopoPatroclo · 2024-02-19T09:43:45Z

For sure.

Current status

If you want to use this library in a typescript project you must use a tsconfig that looks something like this

{
  "compilerOptions": {
    "jsx": "react",
    "jsxFactory": "Html.createElement",
    "jsxFragmentFactory": "Html.Fragment",
    "plugins": [{ "name": "@kitajs/ts-html-plugin" }]
  }
}

the property "jsx": "react" tells typescript to substitute the jsx elements with a call to "Html.createElement" (or "Html.Fragment" in case there is a fragment). This implies that you have to have in your code a declaration of the Html object. The choice is to import it per file or globally using the module @kitajs/html/register as (recommended by the docs).

The catch

When you choose the global import as a way to import the Html object you are going to encounter issues when you are trying to unit test your code (that uses jsx somewhere). This is because you have to import the "register module" on top of every unit test file.
Another case could be made about the pollution of the global object, but I'm not going into detail about that.

Solution

Typescript support "jsx": "react-jsx" that will transpile the jsx code with a call to a function called jsx or jsxs that will be automatically imported from a module named what you configure in jsxImportSource concatenating jsx-runtime. Example:

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "@kitajs/html",
    "plugins": [{ "name": "@kitajs/ts-html-plugin" }]
  }
}

code

const a = <h1>Hello</h1>

transpilled to something like

const runtime = require('@kitajs/html/jsx-runtime')
const a = runtime.jsx('h1', { children: 'Hello' })

As you can see there is no global pollution and the import to the jsx function is made by the transpiler; removing the need to have a global import to @kitajs/html/register module.

The implementation

I've followed the specification expected by typescript during the implementation of the jsx-runtime sub module.
I've created a ts configuration to test that all the current tests work using this different way of configuring typescript/jsx. I had to add "noUnusedLocals": false because all the test import the Html object that is no longer needed with this new config.

Hope this is clearer now, let's discuss about it!

Docs:
https://www.typescriptlang.org/tsconfig#jsx
https://github.com/facebook/react/blob/59831c98cffe0edf706238b067928e7cf54d1065/packages/react/jsx-runtime.js#L4
https://github.com/preactjs/preact/blob/a003d429f4cfa6c131f01eab52d556b242b3fc59/jsx-runtime/src/index.js#L4

arthurfiorette · 2024-02-19T18:16:03Z

When importing other things from @kitajs/html, should we still require the user to create the import just like when importing useState from React, or should we keep the /register-way that, once imported, Html namespace is available everywhere?

Reading more about jsx-runtime, I think we should completely remove the /register import and push the usage of jsx-runtime when global usage is wanted.

README.md

jsx-runtime.d.ts

jsx-runtime.js

package.json

jsx-runtime.js

arthurfiorette · 2024-02-19T18:22:25Z

README.md

@@ -167,7 +182,8 @@ to generate HTML. Here are two options for importing the `@kitajs/html` package:
   ```

 2. **Use the register to add a global namespace**: Import the `register` to globally
-   register all necessary functions for convenience.
+   register all necessary functions for convenience. You don't need to do that if you are
+   using the `jsxImportSource` option in your `tsconfig.json`.


Should we prefer /register import or react-jsx way of "globally" installing kitajs/html? I'm not sure if both would be good.

I think that the best approach is to follow how preact do it https://preactjs.com/guide/v10/typescript/. They let the user choose how to configure Typescript between importing the h and Fragment functions into every file or using the "react-jsx" + "jsxImportSource" approach (automatic import done by the transpiler).

Side note here:
We should also consider adding some documentation on how to setup a project using babel or similar for non-typescript users.

I'm up with you in this one. Let's remove /register and only document manual import via jsx: react or automatic import via jsx: react-jsx.

arthurfiorette · 2024-02-19T18:26:44Z

I've seen jsxDEV and __jsxDEV exports in old react project sources, should we also provide them?

I knew about react-jsx before but could not find any actual spec or implementation description to follow a standard here. That's why I never followed the path and implemented it... Is there something we could validate against?

arthurfiorette · 2024-02-19T18:33:11Z

Additionally, we could drastically improve kitajs/html performance with support for precompiling JSX.

Read more:

oven-sh/bun#6878
https://deno.com/blog/v1.38#fastest-jsx-transform
https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#motivation

JacopoPatroclo · 2024-02-19T21:24:45Z

I've seen jsxDEV and __jsxDEV exports in old react project sources, should we also provide them?

I knew about react-jsx before but could not find any actual spec or implementation description to follow a standard here. That's why I never followed the path and implemented it... Is there something we could validate against?

I don't think that this package need __jsxDEV because it does not have an actual need for it. The __jsxDEV is used to run checks during development on, for example, the fact that you are mapping elements without providing a key prop. Or other stuff like that.

I had the same problem trying to create this first draft. The implementation that I did was against the tests using the alternative typescript config. There is a lot of React-related content on the topic of jsx on the internet but very little on "server side" jsx runtimes. I guess that we should start writing our stuff driven by trial and error.

JacopoPatroclo · 2024-02-19T21:28:36Z

Additionally, we could drastically improve kitajs/html performance with support for precompiling JSX.

Read more:

oven-sh/bun#6878 https://deno.com/blog/v1.38#fastest-jsx-transform https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#motivation

I'm on it

arthurfiorette · 2024-02-22T12:28:48Z

Additionally, we could drastically improve kitajs/html performance with support for precompiling JSX.

Read more:

oven-sh/bun#6878 https://deno.com/blog/v1.38#fastest-jsx-transform https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#motivation

It looks like this is still only supported in Deno + Preact, it's a good idea, but it won't be used by anyone until support arrives later in TS or in packagers. I created a proposal at microsoft/TypeScript#57468 for this, but I'm not sure if it will be accepted by the community...

Until then, this feature will have to wait.

arthurfiorette · 2024-02-22T19:22:51Z

We need to keep the same performance for createElement and jsx/s. Thus, having more functions calls drastically decreased performance, thus I reimplemented createElement for jsxs (children: Children[]) and jsx (children?: Children).

arthurfiorette · 2024-02-22T19:24:52Z

I also simplified JSDoc comments for createElement functions, as their typings were wrong.

arthurfiorette · 2024-02-23T15:05:38Z

Current jsx implementation is still slower

arthurfiorette · 2024-02-23T16:08:06Z

jsx is only used with no children or a single one, this allows us to replace the contentsToString call which has a loop to a single if/else branch.

I also improved the original contentsToString because it previously didn't serialized false and NaN, which was probably a bug.

arthurfiorette · 2024-02-23T16:14:40Z

With react-jsx, we can safely type the Children attribute, previously it was any. This removes even more checks inside contentsToString and move them to the TSC compiler:

<div>{{a:1}}</div> // Previously was fine and was transformed to [object Object], which is unintended

now:

// ts(2353): Object literal may only specify known properties, and 'a' does not exist in type 'Promise<Children> | Children[]'
<div>{{a:1}}</div>

Increasing even further the performance

arthurfiorette · 2024-02-23T16:35:51Z

jsx-runtime is faster than createElement (That's probably why react itself moved to it). I think we should make it default and deprecate the createElement way.

arthurfiorette · 2024-02-23T17:14:51Z

Current tasks:

add jsx and jsxs jsdoc
Move documentation to be react-jsx first
Fix serialization test (false now is "false" instead of ""...)
Add a table containing input -> outputs to be expected in the readme, like true -> "true", [1,"3"] -> "13" and so on... (Functions and objects will throw tsc errors)
Ensure everything keeps working with ts-html-plugin.
Add a way to deprecate older createElement/import Html way
Fix benchmarks to use newer jsxs syntax for kita

JacopoPatroclo · 2024-02-24T12:09:13Z

Current tasks:

add jsx and jsxs jsdoc

Move documentation to be react-jsx first

Fix serialization test (false now is "false" instead of ""...)

Add a table containing input -> outputs to be expected in the readme, like true -> "true", [1,"3"] -> "13" and so on... (Functions and objects will throw tsc errors)

Ensure everything keeps working with ts-html-plugin.

Add a way to deprecate older createElement/import Html way

Fix benchmarks to use newer jsxs syntax for kita

@arthurfiorette let me know your thoughts about the deprecation message I add to the register module. Moreover, I think that adding the @deprecated jsdoc property to the createElement function does not make a lot of sense since it is usually used by the transpiler and not directly by the end user. We should make it explicit in the docs and at some point in the future substitute the functionality with a throw Error that explains how to migrate and where to find the documentation to do it.

I will also add to the tasks:

Migrate tests to utilize the jsx-runtime instead of the createElement function (this will increase the code coverage also I think)

I will handle the benchmarks now.

arthurfiorette · 2024-02-24T18:13:22Z

I improved even further JSX/JSXS performance, using the average of kb/html and the time per iteration of our benchmarks, this is the result:

Holy shit 1.2gbps

I can finally rest in peace now XD.

https://github.com/fastify/process-warning is a great example on how to deprecate things, so I followed their pattern (without installing it as a dependency).

This is how it shows up on bun and nodejs:

And this is what the user will see after clicking the link:

I also added more examples to the serialization table.

arthurfiorette · 2024-02-24T18:18:13Z

@JacopoPatroclo could you finish these two tasks below?

Ensure everything keeps working with ts-html-plugin.
Fix benchmarks to use newer jsxs syntax for kita

@arthurfiorette let me know your thoughts about the deprecation message I add to the register module. Moreover, I think that adding the @deprecated jsdoc property to the createElement function does not make a lot of sense since it is usually used by the transpiler and not directly by the end user. We should make it explicit in the docs and at some point in the future substitute the functionality with a throw Error that explains how to migrate and where to find the documentation to do it.

I think only using the correct way to deprecate it (process warning) is enough for now, let's see how the users react to it.

I will also add to the tasks:

Migrate tests to utilize the jsx-runtime instead of the createElement function (this will increase the code coverage also I think)

Awesome!

README.md

JacopoPatroclo · 2024-02-25T22:27:17Z

@arthurfiorette I'm encountering issues migrating the benchmarks to use the new JSX-runtime.
The main pinpoint is related to the fact that the benchmarks are written using es-modules but the package uses commonjs.
This should not be an issue but the jsx runtime import is transpilled by typescript like this:

import { jsx as _jsx, jsxs as _jsxs } from '../../jsx-runtime'

but as stated from the Node.js docs you can only import common-js modules using a "global import"

import something from 'common-js-module'

I've tried to convert everything to common-js but the package that we are using for benchmarking (mitata) seems to only work when imported using esm syntax. It's kind of a mess. I'm not sure how to bring home this refactor.

Side note, we should consider using pnpm-workspace to handle the benchmarks piece, it would be nice to reference the jsx-runtime using the real path (@kitajs/htm) instead of something like (../..). This could be achieved by creating a project that has between it's dependencies @kitajs/htm with the version workspace:*

Let me know what you think of my reasoning above and about the workspace approach.

arthurfiorette · 2024-02-25T23:12:05Z

Let me know what you think of my reasoning above and about the workspace approach.

You could try to self install using a file path at benchmark/package.json:

{
  "type": "module",
  "dependencies": {
    "kitajs": "file:.." 
  }
}

Regarding the workspace approach, I'll probably create a workspace to group all html related projects into a single repository:

https://github.com/kitajs/html
https://github.com/kitajs/fastify-html-plugin
https://github.com/kitajs/ts-html-plugin

But I do not have time for this now, maybe after this PR gets merged I can work on it.

JacopoPatroclo · 2024-02-28T14:54:25Z

@arthurfiorette let me know if my refactoring of the project suits you.
I've separated each renderer into its package, now we have a cleaner way to handle the ts compilation for each package.
The benchmark runner has the same checks and bench as before to ensure that each renderer is going to output the same markup as @kitajs/html.

JacopoPatroclo · 2024-02-28T14:56:21Z

Let me know what you think of my reasoning above and about the workspace approach.

You could try to self install using a file path at benchmark/package.json:
{
  "type": "module",
  "dependencies": {
    "kitajs": "file:.." 
  }
}
Regarding the workspace approach, I'll probably create a workspace to group all html related projects into a single repository:

https://github.com/kitajs/html https://github.com/kitajs/fastify-html-plugin https://github.com/kitajs/ts-html-plugin

But I do not have time for this now, maybe after this PR gets merged I can work on it.

I can help with this task, I'm very familiar with Nx and pnpm workspaces.

arthurfiorette · 2024-03-02T20:49:36Z

I've added changesets, before commiting any feature, please run pnpm changeset, choose all changed packages by your feature and add a release note to it. Changesets will automatically bump versions, create a GH release and a changelog.md

arthurfiorette · 2024-03-02T20:51:56Z

Tasklist:

Update links and references inside all package jsons.
Ensure tests are working on all packages
Test CI/CI
Test changeset
Update docs and reference links
Do a simple publish --dry-run to ensure all files are being bundled correctly
See if there's no missing or left configuration files
Update monorepo readme's
Create a CONTRIBUTING guide to teach how to use pnpm changeset before commit.

Regarding ts-html-plugin I'm not sure what's going on with these tests, but its probably related to the @kitajs/html types, as when I change "@kitajs/html": "workspace:^" (linked locally) to "@kitajs/html": "3.1.1" (downloaded from npm), tests starts to pass.

…options to tsserver on tests, align test script tag to execute test with the build

JacopoPatroclo · 2024-03-04T10:40:34Z

Tasklist:

Update links and references inside all package jsons.

Ensure tests are working on all packages

Test CI/CI

Test changeset

Update docs and reference links

Do a simple publish --dry-run to ensure all files are being bundled correctly

See if there's no missing or left configuration files

Update monorepo readme's

Create a CONTRIBUTING guide to teach how to use pnpm changeset before commit.

Regarding ts-html-plugin I'm not sure what's going on with these tests, but its probably related to the @kitajs/html types, as when I change "@kitajs/html": "workspace:^" (linked locally) to "@kitajs/html": "3.1.1" (downloaded from npm), tests starts to pass.

I've figured out the issue related ts-html-plugin tests not passing using the kitajs/html workspace version. Debugging the ts-server was a pain. Hope the fix is good enough. The issue was a mix of things, starting from how pnpm links packages to how typescript resolves them in different ways depending on the status of the file system.

JacopoPatroclo · 2024-03-04T10:43:41Z

packages/ts-html-plugin/test/unsafe-tags.test.ts

@@ -11,7 +11,6 @@ it('Detect xss prone usage', async () => {
      <>
        <div>
        <div>{html}</div>
-        <div>{object}</div>


@arthurfiorette I'm not sure about this, empty Objects are no longer supported by kitajs/html, right? Should we live this line by adding an expected ts error or should we, as I did, remove the line completely?

Hmm, but the error thrown in this line is not from the ts-html-plugin and just a type error from typescript.

Yep, that's correct. So we should not test this behavior. I'll leave it like this.

arthurfiorette · 2024-03-04T15:08:41Z

I've figured out the issue related ts-html-plugin tests not passing using the kitajs/html workspace version. Debugging the ts-server was a pain. Hope the fix is good enough.

For the love of god thank you!!! ts-server is a MAJOR pain.

arthurfiorette · 2024-03-05T17:28:17Z

I'll review it asaic. In the meantime, this article have been sent to me regarding closing tags. Should we also remove it from kitajs/html?

arthurfiorette · 2024-03-09T21:50:25Z

In the meantime, this article have been sent to me regarding closing tags. Should we also remove it from kitajs/html?

React still uses self closing tags, so let's just keep it in this way.

arthurfiorette · 2024-03-09T21:51:28Z

I think were ready to merge. @JacopoPatroclo is there anything else missing?

JacopoPatroclo · 2024-03-10T01:17:23Z

I think were ready to merge. @JacopoPatroclo is there anything else missing?

@arthurfiorette I think we should do a release candidate of the packages to test them inside some real projects. Just to make sure that they all are working properly. Can you manage to do that? I will test them on some of my projects as well. After that, we can close this PR.

If you like I could add Nx to enable caching and task orchestration to the monorepo. Of course, I will do it in a separate PR. Is there a place where we can discuss stuff like this outside of PR's comments?

arthurfiorette · 2024-03-10T21:42:03Z

I think we should do a release candidate of the packages to test them inside some real projects.

Me too, I'll just merge this to main because the changeset is set up there...

If you like I could add Nx to enable caching and task orchestration to the monorepo.

Pnpm already does caching, task orchestration isn't needed for now as no complicated build step is required. Changesets already does releasing stuff too, so I think its enough for now.

arthurfiorette · 2024-03-10T21:42:53Z

Anyways, thanks for the big effort you've put in here :)

arthurfiorette · 2024-03-10T23:56:18Z

Is there a place where we can discuss stuff like this outside of PR's comments?

Yes, add me on discord: @arthurfiorette

arthurfiorette self-assigned this Feb 19, 2024

arthurfiorette added enhancement New feature or request help wanted Extra attention is needed labels Feb 19, 2024

arthurfiorette requested changes Feb 19, 2024

View reviewed changes

JacopoPatroclo force-pushed the master branch from fd9a819 to 11b6a59 Compare February 19, 2024 20:41

kurtextrem reviewed Feb 25, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

arthurfiorette approved these changes Feb 25, 2024

View reviewed changes

arthurfiorette added 2 commits March 2, 2024 17:48

code

73a07f1

packagejson

67ac1d2

arthurfiorette and others added 5 commits March 2, 2024 17:52

ci

c00efaf

lockfile

9f35b92

ci

b3732c2

vscode

b611195

ts-html-plugin: solve path resolution issue on tests, add debug file …

a9a765e

…options to tsserver on tests, align test script tag to execute test with the build

JacopoPatroclo force-pushed the master branch from 42cd262 to a9a765e Compare March 4, 2024 10:32

JacopoPatroclo commented Mar 4, 2024

View reviewed changes

arthurfiorette added 3 commits March 7, 2024 23:28

refactor: renamed testing env var to a more descriptive one

26fe409

fix: test inconsistencies

1d96f49

merge: merge main

aa8973d

arthurfiorette approved these changes Mar 9, 2024

View reviewed changes

arthurfiorette added 2 commits March 9, 2024 20:37

chore: declaration maps for benchmarks

7a51612

chore: changeset

f4656f4

arthurfiorette merged commit f6a8c11 into kitajs:master Mar 10, 2024
3 checks passed

arthurfiorette mentioned this pull request Mar 10, 2024

Release plan (next) #138

Merged

This was referenced Apr 3, 2024

Release plan #173

Merged

Release plan #175

Closed

Add support for jsx-runtime #129

Add support for jsx-runtime #129

Conversation

JacopoPatroclo commented Feb 18, 2024

codecov bot commented Feb 18, 2024 • edited Loading

Codecov Report

arthurfiorette commented Feb 18, 2024

JacopoPatroclo commented Feb 19, 2024

Current status

The catch

Solution

The implementation

arthurfiorette commented Feb 19, 2024

arthurfiorette Feb 19, 2024

Choose a reason for hiding this comment

JacopoPatroclo Feb 19, 2024

Choose a reason for hiding this comment

arthurfiorette Feb 22, 2024

Choose a reason for hiding this comment

arthurfiorette commented Feb 19, 2024

arthurfiorette commented Feb 19, 2024

JacopoPatroclo commented Feb 19, 2024

JacopoPatroclo commented Feb 19, 2024

arthurfiorette commented Feb 22, 2024

arthurfiorette commented Feb 22, 2024

arthurfiorette commented Feb 22, 2024

arthurfiorette commented Feb 23, 2024

arthurfiorette commented Feb 23, 2024

arthurfiorette commented Feb 23, 2024

arthurfiorette commented Feb 23, 2024

arthurfiorette commented Feb 23, 2024 • edited Loading

JacopoPatroclo commented Feb 24, 2024 • edited Loading

arthurfiorette commented Feb 24, 2024

arthurfiorette commented Feb 24, 2024

JacopoPatroclo commented Feb 25, 2024

arthurfiorette commented Feb 25, 2024 • edited Loading

JacopoPatroclo commented Feb 28, 2024

JacopoPatroclo commented Feb 28, 2024

arthurfiorette commented Mar 2, 2024

arthurfiorette commented Mar 2, 2024

JacopoPatroclo commented Mar 4, 2024

JacopoPatroclo Mar 4, 2024

Choose a reason for hiding this comment

arthurfiorette Mar 4, 2024

Choose a reason for hiding this comment

JacopoPatroclo Mar 4, 2024

Choose a reason for hiding this comment

arthurfiorette commented Mar 4, 2024

arthurfiorette commented Mar 5, 2024

arthurfiorette commented Mar 9, 2024

arthurfiorette commented Mar 9, 2024

JacopoPatroclo commented Mar 10, 2024

arthurfiorette commented Mar 10, 2024

arthurfiorette commented Mar 10, 2024

arthurfiorette commented Mar 10, 2024

codecov bot commented Feb 18, 2024 •

edited

Loading

arthurfiorette commented Feb 23, 2024 •

edited

Loading

JacopoPatroclo commented Feb 24, 2024 •

edited

Loading

arthurfiorette commented Feb 25, 2024 •

edited

Loading