BuilderIO / mitosis

Write components once, run everywhere. Compiles to React, Vue, Qwik, Solid, Angular, Svelte, and more.
https://mitosis.builder.io
MIT License
11.87k stars 518 forks source link

Mitosis JSX types are not properly mapped #461

Open samijaber opened 2 years ago

samijaber commented 2 years ago

Originally from https://github.com/BuilderIO/mitosis/issues/372#issuecomment-1150135164

For this input:

import { HTMLAttributes } from "react";

export const headingSizes = [1, 2, 3, 4, 5, 6];

export interface HeadingProps extends HTMLAttributes<HTMLHeadingElement> {
  size: typeof headingSizes[number];
}

export default function Heading(props: HeadingProps) {
  return <div />
}

the output does not preserve the import { HTMLAttributes } from "react"; import

samijaber commented 2 years ago

cc @originswift-sys

PatrickJS commented 2 years ago

I saw @kylecordes use this. can we update the imports to remove the need for dist/src

import '@builder.io/mitosis/dist/src/jsx-types';
PatrickJS commented 2 years ago

we need docs on how to import jsx-types and how to extend the types

samijaber commented 2 years ago

By the way @originswift-sys, your issue might be tied to the fact that we compile away React imports:

https://github.com/BuilderIO/mitosis/blob/454dc3f44335a4b6bcb6d085f51a389dc666b2c3/packages/core/src/parsers/jsx.ts#L1013-L1026

Will get back to this issue with more details later, and information on providing type imports.

originswift-sys commented 2 years ago

@samijaber not just the react imports though... none of my exports are included in the output. I'm currently blocked because I can't import shared logic and theme components. I was also wondering if it would be okay to preserve direct css imports (verbatim), e.g. import "base-styles.css"; and allow the processor installed in the consuming project handle that bit. Maybe as a configurable option?

originswift-sys commented 2 years ago

I thought about several additional ways of transforming the output. Like lambda hooks: afterGen(targetFramework, ...otherOptions), beforeGen(targetFramework, ...otherOptions), etc... so one could basically use these to include or remove stuff, pre and post generation.

samijaber commented 2 years ago

I thought about several additional ways of transforming the output. Like lambda hooks

We have a plugin framework already that does exactly that: https://github.com/BuilderIO/mitosis/blob/1b58dd30309eb91a3cc27bab1435e52f86097356/docs/customizability.md

@samijaber not just the react imports though... none of my exports are included in the output. I'm currently blocked because I can't import shared logic and theme components.

You're saying exports are not included either? It would be super useful if you could provide the following:

originswift-sys commented 2 years ago

Thanks for pointing me to the plugin. I missed it.

What I mean is, previously, imports were just copied over to the generated output. So if I did:

import { defaultClassMap } from "../constants"

...it was simply copied over to the output. The recent version only deals with exported tokens. That said, when a component references an imported token, the output is broken, because the used references (imports) are left out. The plugin is one way to achieve this (by injecting imports statements at the beginning of outputs), but I preferred the previous behaviour where all imports (excluding mitosis-related) were simply copied over.

Oh, and I'm using the CLI commands.

dystopiandev commented 2 years ago

Bump. Any workarounds for this?

originswift-sys commented 1 year ago

Stuck with a really old version because this is still happening as of latest 😞

samijaber commented 1 year ago

Two things to node:

module.exports = {
  files: 'src/**',
  targets: ['reactNative', 'vue2', 'vue3', 'solid', 'svelte', 'react', 'qwik'],
  options: {
    qwik: {
      typescript: true,
    },
  },
};

Then we should now be preserving TS imports.

import type { JSX } from '@builder.io/mitosis/jsx-runtime';

JSX.HTMLAttributes

or use them directly from the global namespace if you've setup the JSXRuntime source correctly: https://github.com/BuilderIO/mitosis#setup-typescript

R-Bower commented 1 year ago

Two things to node:

  • We have made some big improvements to how we handle TS code recently. If you add typescript: true as an option for your target in mitosis.config.js like:
module.exports = {
  files: 'src/**',
  targets: ['reactNative', 'vue2', 'vue3', 'solid', 'svelte', 'react', 'qwik'],
  options: {
    qwik: {
      typescript: true,
    },
  },
};

Then we should now be preserving TS imports.

  • However, we still have the issue with the React import. As I look at this again, I realize that what you're doing is an anti-pattern: you should not be importing anything from 1 specific framework into your Mitosis code (even types). Otherwise, you will end up with React types in Vue, Svelte, Qwik, etc. which would not make any sense. For this particular case, you should import the HTMLAttributes interface from Mitosis' own JSX Runtime types. You can import them directly:
import type { JSX } from '@builder.io/mitosis/jsx-runtime';

JSX.HTMLAttributes

or use them directly from the global namespace if you've setup the JSXRuntime source correctly: https://github.com/BuilderIO/mitosis#setup-typescript

Is this the recommended approach for every lib? I've tried this with a button element and the React output is throwing type errors. JSX.HTMLAttributes<HTMLButtonElement> isn't equivalent to React.HTMLAttributes<HTMLButtonElement>.

samijaber commented 1 year ago

Is this the recommended approach for every lib? I've tried this with a button element and the React output is throwing type errors. JSX.HTMLAttributes<HTMLButtonElement> isn't equivalent to React.HTMLAttributes<HTMLButtonElement>.

Hmm, I see. The issue here is that the Mitosis JSX types will never perfectly match the React JSX types (or the component types of any other framework for that matter). If you can share some concrete examples of code that runs into this issue, that'll help us dig into what the best solution is.

The primary solution that comes to mind at the moment is to have Mitosis replace every import of the JSX types with the equivalent types for each framework. Even then, I'm curious if this will work for all scenarios, but might be worth trying it out and seeing how far it can go. 🤔

R-Bower commented 1 year ago

@samijaber this is immediately a problem with every event handler.

import {JSX} from "@builder.io/mitosis/jsx-runtime"

export default function ButtonComponent(
  props: JSX.HTMLAttributes<HTMLButtonElement>,
) {
  return (
    <button class={"root"} {...props}>
      <div class={"content"}>{props.children}</div>
    </button>
  )
}

If you inspect the React output for this type, you'll see that there are dozens of issues. Most of the event handlers of the JSX namespace are incompatible with the event handlers for the corresponding React element.

This is a pretty big blocker. Since I'm using mitosis for mostly leaf elements like buttons, text, etc... I need to be able to spread props to the base element + produce accurate types in each output target.

For React it's probably as simple as replacing JSX. with React., as that'd produce the correct type (ex: React.HTMLAttributes<HTMLButtonElement> works as expected). I can't speak to the other frameworks though.

R-Bower commented 1 year ago

Did some thinking on this... I've solved this, at least for the React output, by using a custom replacer post-compilation. All of my component's types are in a file matching the component's name, e.g. button.types.ts, so it's pretty easy for me to find+replace. Not sure how this might make its way into the core library, but it's definitely worth some consideration. There needs to be a way to accurately reflect all of a component's props for each output target.

  function typesFormatter(params: TypesFormatterParams) {
    const {fileData} = params
    let data = fileData
    const typeImport = `import type {JSX} from "@builder.io/mitosis/jsx-runtime"`
    if (data.includes(typeImport)) {
      data = data.replace(typeImport, "").replace(/\n*/, "")
      if (!data.includes(`import * as React from "react"`)) {
        data = [`import * as React from "react"`, data].join("\n")
      }
      return data.replace("JSX.", "React.")
    }
    return data
  }
samijaber commented 1 year ago

Thanks @R-Bower, that's informative. You're right, we need a way to map the Mitosis JSX types to the correct framework types, or folks will run into many type-related issues.

There is definitely a way to transform the import to that of React's JSX types, it will look something like this:

Step 1 - Identify every place where the JSX types are used

in the JSX parser, we will need to update the logic that parses import declarations here: https://github.com/BuilderIO/mitosis/blob/3717ef34ace920f725f0ceebb61b26c5e61b661a/packages/core/src/parsers/jsx/imports.ts#L7-L44

such that: if we see a JSX import coming from @builder.io/mitosis or @builder.io/mitosis/jsx-runtime, we want to store that import (currently we ignore all mitosis imports). Might be good to normalize all possible variations (regular import, type import, import from different paths) into 1 single variant: import type { JSX } from '@builder.io/mitosis/jsx-runtime';

We will also need to

Step 2 - transform imports

This might be the best place to make such a change: https://github.com/BuilderIO/mitosis/blob/3717ef34ace920f725f0ceebb61b26c5e61b661a/packages/cli/src/build/helpers/transpile.ts#L15-L30

This import transform code will run for both mitosis components (.lite.tsx) and plain JS/TS files. If will want to replace the normalized import type { JSX } from '@builder.io/mitosis/jsx-runtime'; import statement to whatever makes sense for each given framework. For React, It will be easiest to convert the import to:

type JSX  = React;

This will avoid the need to replace every JSX.* within the code with React.. This assumes that import React from 'react'; already exists somewhere in the code (which it does as we always add it to react generators). But if it somehow doesn't, we could add a check and add it ourselves.

The solution will be different for each framework, but should roughly follow this same pattern of changes.

rockitweb commented 1 year ago

Hi @samijaber,

I couldn't see if this had been taken on by anyone so I thought I'd have a go based on your suggested approach above. I'm particulary focussed on React and Qwik. The React side seems straight forward(ish) it's the Qwik side that's hurting me.

Say we have the following import and type declaration:

// .lite.tsx
import { JSX } from '@builder.io/mitosis/jsx-runtime';

type Props = JSX.AnchorHTMLAttributes<HTMLAnchorElement>;
...

For React we would want something like: As previously stated switching out the JSX with React

import * as React from "react";

type Props = React.AnchorHTMLAttributes<HTMLAnchorElement>;

But for the correct types to be declared in Qwik it needs to be:

import {QwikJSX} from "@builder.io/qwik";

type props = QwikJSX.IntrinsicElements["a"]

I guess I have two questions:

  1. Am I missing something with qwik RE html types? I've tried to find alternate ways and just can't come up with one
  2. If this is the correct way in qwik the only solution I can come up with is to do some type of mapping. Thoughts?