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

[JSX] Print Empty Elem - Self closing tags when elem has no children #4037

Merged
merged 5 commits into from
Feb 9, 2025

Conversation

shayanhabibi
Copy link
Contributor

@shayanhabibi shayanhabibi commented Feb 8, 2025

I'm trying to enable and enforce the JSX print to self close tags when they don't have children. This way property spreads or children attributes aren't overridden in the Oxpecker.Solid dsl.

Draft:

  • I'd really appreciate any direction to where tests for this would be contained and how to further this pull request to completion.
  • I've done my best to grok the origin of this behaviour and treat it. Apologies if this is incorrect. I would appreciate guidance.

@alfonsogarciacaro
Copy link
Member

I don't remember if there was a specific reason not to close the tag immediately but probably it should be fine. Do we need a space before />? It'd be great to have some JSX tests in tests/Integration/data

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 8, 2025

Well @Lanayx identified that it is not accepted under the HTML5 spec; however it is in JSX/React etc. Without this, spreading properties that contain children are overridden without explicit children between the open and closing tags.

Since this transformation only applies to JSX, I hope it would be fine. If the functionality is being used to template HTML5, then it might cause issue. Most browsers accept it anyway, but there can be undefined behaviour in some instances.

I can try and grok the tests and put something together, but don't know whether they'll be up to scratch

@alfonsogarciacaro
Copy link
Member

Thanks for the explanation. Could you please add a succinct version as a comment to the code. This way if in the future we want to generate the closing tag to conform with HTML5 we'll remember there's a reason to do it like this.
The CounterJSX component is being called without children in the React tests, so at least that should be a way to test the feature.

@shayanhabibi
Copy link
Contributor Author

shayanhabibi commented Feb 8, 2025

Thanks for having a look, your work on this project, and for the helpful comments!

I've added the documentation with some source references but will have to defer writing up the tests to tomorrow for this and #4038 .

For the time being, there's nothing obstructing me since I can use a local build of fable with some confidence the functionalities will be merged in some way.

@MangelMaxime MangelMaxime merged commit 5c95930 into fable-compiler:main Feb 9, 2025
19 checks passed
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.

3 participants