Add support for jsx-runtime

[JacopoPatroclo]

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]

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]

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

[JacopoPatroclo]

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]

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.

[arthurfiorette]

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

[JacopoPatroclo]

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.

[arthurfiorette]

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]

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]

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]

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]

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]

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]

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]

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

[arthurfiorette]

Current jsx implementation is still slower

[arthurfiorette]

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]

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]

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]

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]

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]

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]

@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!

[JacopoPatroclo]

@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]

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]

@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]

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]

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]

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.

---

[JacopoPatroclo]

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]

@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?

[arthurfiorette]

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

[JacopoPatroclo]

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

[arthurfiorette]

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]

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]

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]

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

[JacopoPatroclo]

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]

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]

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

[arthurfiorette]

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

Yes, add me on discord: @arthurfiorette