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: fix ref issue when transforming to Solid.js code #1384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rqzheng2015
Copy link
Contributor

@rqzheng2015 rqzheng2015 commented Mar 18, 2024

Description

Fix ref issue when mitosis build target is solid

Issue

I used the ref example provided on the official doc.

Playground: Click here

It is correctly transformed on React, which will present the output codes like these below:

import * as React from "react";
import { forwardRef } from "react";

const MyInput = forwardRef(function MyInput(props, inputRef) {
  return <input ref={inputRef} />;
});

export default MyInput;

But, it will cause issue on Solid, and we could see the issue at console panel.

image

Reason

The issue is caused by the getRefsString method in Solid.js generator, which can be read here.

const getRefsString = (json: MitosisComponent, options: ToSolidOptions) =>
  Array.from(getRefs(json))
    .map((ref) => {
      const typeParameter = (options.typescript && json['refs'][ref]?.typeParameter) || '';
      return `let ${ref}${typeParameter ? ': ' + typeParameter : ''};`;
    })
    .join('\n');

And with the insertion ${getRefsString(json, options)} in solid code, eventaully, the generated solid.js code will become something like this:

function MyComponent(props:any) {
  let props.inputRef;
  return (
    <input  ref={props.inputRef!}  />
    )
}
export default MyComponent;

And when these code met with prettier format function in Solid.js generator, it will throw out the error, which leads to the break of Solid.js code bundle.

We could easily reproduce it in a online prettier editor.
image

Solution

Updated at May, 4th:

Remove ref definition when it's passed by props, such as props.inputRef.

const getRefsString = (json: MitosisComponent, options: ToSolidOptions) =>
  Array.from(getRefs(json))
    .map((ref) => {
      if (ref.includes('.')) return '';
      const typeParameter = (options.typescript && json['refs'][ref]?.typeParameter) || '';
      return `let ${ref}${typeParameter ? ': ' + typeParameter : ''};`;
    })
    .join('\n');

Then it will behave in two ways:

  • Ref is created by useRef, will still remain.
  • Ref is passed by props, will not remain after this PR.

Remove the getRefsString method calling.

Then the Solid.js generator will generae Solid.js code like these below, which works as expected and will not throw out any error.

function MyComponent(props:any) {
  return (
    <input ref={props.inputRef!}  />
    )
}

export default MyComponent;

Copy link

changeset-bot bot commented Mar 18, 2024

🦋 Changeset detected

Latest commit: a1ca5ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@builder.io/mitosis Patch
@builder.io/mitosis-cli Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented Mar 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mitosis-fiddle ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 18, 2024 11:50am

@rqzheng2015 rqzheng2015 changed the title fix: fix ref issue when transforming to solid.js code fix: fix ref issue when transforming to Solid.js code Mar 18, 2024
Copy link

nx-cloud bot commented Mar 18, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a1ca5ad. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

@rqzheng2015
Copy link
Contributor Author

rqzheng2015 commented Mar 18, 2024

Hi, @samijaber. Could you help me with the pr test failed issue?

I've been tested on my local macOS and update snapshot, and it all worked well. But the test just failed on github action, any particular reason why? Thanks.

Same test failed issue happens on my other PR as well.

@rqzheng2015
Copy link
Contributor Author

Hi, @samijaber, do you have a moment to see the reason of why vitest snapshot not correctly generated? That would be great help for my MR, thanks.

@samijaber
Copy link
Contributor

Hi @rqzheng2015 , sorry for the delay. There's an issue with our test suite causing tests to not work properly.

It is causing all PRs to break unless additional instructions are followed.

I will fix this issue first and then let you know to rerun your tests locally to get them fixed

@@ -61,14 +60,6 @@ function getContextString(component: MitosisComponent, options: ToSolidOptions)
return str;
}

const getRefsString = (json: MitosisComponent, options: ToSolidOptions) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not the right fix. you are getting rid of all ref declarations/

what we need to do is fix getRefs so it does not include ref attributes whose contents are javascript expressions.

if (typeof item.bindings.ref?.code === 'string') {
refs.add(item.bindings.ref.code);
}
}

an extra check needs to be made here, maybe exclude anything with a . as a quick fix for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sorry for the late response. I have corrected it in the getRefsString method of Solid.js generator without removing the getRefsString method itself. Please check again, thanks.

@rqzheng2015
Copy link
Contributor Author

Hi @samijaber, I have modified my PR to fix the ref issue changes and vitest snapshot issues, please check when you are free, thanks.

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.

None yet

2 participants