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(eslint): Allow JSXNode to serialize #5883

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

genki
Copy link
Contributor

@genki genki commented Feb 23, 2024

Overview

Allow the JSXNode to serialize.

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

Even though there is the JSXNodeSerializer, the eslint has been warning when you use the JSXNode across the $ sign.
This PR solves this issue.

Use cases and why

Fixes #5864

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Copy link

netlify bot commented Feb 23, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 0ea5eb3

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

Are you sure that this approach is enough? I would have thought that it needs changing here https://github.com/BuilderIO/qwik/pull/5883/files#diff-63ce4c4f15293afb89991dad20de59170680fe6145e0188174aea7365efed3b4R336

@genki
Copy link
Contributor Author

genki commented Feb 23, 2024

@wmertens I am not sure, I couldn't come up with the issue.
I have tested for JSXNode, JSXOutput and ReturnType<typeof jsx> and got no warnings.
Are there something I have to think other than those types?
The JSXNode contains callable members, but the _isTypeCapturable function seemed to return true, if the type is once detected as an instance of JSXNodeImpl, without checking its inside.

@wmertens wmertens merged commit dc30393 into QwikDev:main Feb 23, 2024
22 checks passed
@PatrickJS
Copy link
Member

yeah I needed this

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

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

Thanks @genki
We appreciate a lot your help 😎

@genki genki deleted the serialize_jsxnode branch February 24, 2024 11:11
@wmertens
Copy link
Member

This will be available soon as the dev version on npm, please test.

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.

[🐞] Warned when serializing JSXNode.
4 participants