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

Fix JSX runtime #1519

Merged
merged 1 commit into from
Feb 5, 2023
Merged

Fix JSX runtime #1519

merged 1 commit into from
Feb 5, 2023

Conversation

wooorm
Copy link
Contributor

@wooorm wooorm commented Feb 3, 2023

Summary

This practically fixes the JSX development runtime, which passes several other parameters, which currently results in Solid not rendering nodes.

It also fixes a case where a key parameter might be passed as undefined. This does not happen normally when using a compiler that compiles JSX to function calls, because Solid as far as I understand does not support keys. However, as the key is defined in the API of jsx, I think it’s better to cut it off.

How did you test this change?

With the following code in a browser:

/* eslint-env browser */
import h from 'https://esm.sh/solid-js@1/h'
import {jsx, jsxDEV} from 'https://esm.sh/solid-js@1/h/jsx-runtime'
import {render} from 'https://esm.sh/solid-js@1/web?dev'

function jsxFixed(type, props) {
  return h(type, props)
}

render(ComponentA, document.getElementById('root'))
// Renders `<div>false</div>`
// Fixed: `<div>asd</div>`

render(ComponentB, document.getElementById('root'))
// Renders `<div></div>`
// Fixed: `<div>asd</div>`

function ComponentA() {
  return jsxDEV(
    'div',
    {children: 'asd'},
    undefined,
    false,
    {
      fileName: 'example.js',
      columnNumber: undefined,
      lineNumber: undefined
    },
    undefined
  )
}

function ComponentB() {
  return jsx('div', {children: 'asd'}, undefined)
}

function ComponentAFixed() {
  return jsxFixed(
    'div',
    {children: 'asd'},
    undefined,
    false,
    {
      fileName: 'example.js',
      columnNumber: undefined,
      lineNumber: undefined
    },
    undefined
  )
}

function ComponentBFixed() {
  return jsxFixed('div', {children: 'asd'}, undefined)
}

This practically fixes the JSX development runtime, which passes several
other parameters, which currently results in Solid not rendering nodes.

It also fixes a case where a key parameter might be passed as `undefined`.
This does not happen normally when using a compiler that compiles JSX to
function calls, because Solid as far as I understand does not support keys.
However, as the key is defined in the API of `jsx`, I think it’s better to
cut it off.
@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

⚠️ No Changeset found

Latest commit: c479589

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ryansolid
Copy link
Member

Oh so the new transform doesn't do the 3rd and n... arguments as children. I didn't realize that. Thanks for pointing this out.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4082978084

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.251%

Totals Coverage Status
Change from base Build 4078768340: 0.0%
Covered Lines: 1296
Relevant Lines: 1393

💛 - Coveralls

@ryansolid ryansolid merged commit deed4d6 into solidjs:main Feb 5, 2023
@wooorm wooorm deleted the fix-jsx-runtimes branch February 5, 2023 12:43
@wooorm
Copy link
Contributor Author

wooorm commented Feb 5, 2023

Yeah, the new transform moves a lot of the lifting to the compilation phase, so you don’t need to do that at runtime anymore.

For example, if there is zero or one child, it will now be passed as {children: Child | undefined}, to jsx. But if there are multiple, you’ll get them as {children: Array}tojsxs`.
You could used that for potential performance improvements.

In the dev runtime, you’ll also get as shown above useful extra values, such as in which file and at what line/column the thing was authored, which could be used to improve devtools.

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

Successfully merging this pull request may close these issues.

4 participants