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

Invalid JSX code generation when nesting <a> elements #2279

Closed
nbrugger-tgm opened this issue Sep 10, 2024 · 5 comments
Closed

Invalid JSX code generation when nesting <a> elements #2279

nbrugger-tgm opened this issue Sep 10, 2024 · 5 comments

Comments

@nbrugger-tgm
Copy link

nbrugger-tgm commented Sep 10, 2024

Describe the bug

In certain scenarios the code generated from JSX is incorrect. It tries to use nextSibling when the object has no sibling which causes null pointer exceptions (undefined)

Your Example Website or App

https://playground.solidjs.com/anonymous/adc53903-ec95-4c45-a640-5dc2aa7baf15

Steps to Reproduce the Bug or Issue

  1. Go to the playground provided

Expected behaviour

When JSX that is forbidden/invalid is transpiled/transformed an error should be thrown (compile time) that enables the user to fix his JSX.

Platform

  • OS: Linux
  • Browser: Chrome
  • Version: 126.0.6478.126

Additional context

solidjs/solid-start#1622

@nbrugger-tgm
Copy link
Author

Additional information & help thread: https://discord.com/channels/722131463138705510/1283002130273140746

The issue is <a> objects being nested and the JSX compiler ignores that and generates wrong js code; and doesn't provide a usefull error message

@nbrugger-tgm nbrugger-tgm changed the title Invalid JSX code generation (causes crash cannot access classList of undefined) Invalid JSX code generation when nesting <a> elements Sep 10, 2024
@ryansolid
Copy link
Member

There are some checks in place we use a package(https://github.com/MananTank/validate-html-nesting) but it only checks direct descendants. The tricky part is the DOM has dozens if not hundreds of these exception cases. We've tried several different approaches to detecting them, including template resolution comparison, tag counting, and this current children check.

It seems that there is a different class of errors where elements can't be children at any depth. Like <a> can never be a descendant of another <a>. I wonder how many other cases are like that.

For SSR this is even more interesting problem since cross template considerations come into play as it all gets dumped into a single HTML output: #2274

@nbrugger-tgm
Copy link
Author

nbrugger-tgm commented Sep 10, 2024

Can the browser deal with nested <a>s if yes then i dont understand why the error occurs

If no then it would be reasonable to try to utilize the browser to provide a useful message. Depending on how the browser erros when nesting s solid could catch that and then reword/enrich it with sourcemaps and rethrow (and ofc this also means treating all elements the same for JSX transform

but you are far more qualified than me obviously and i am sure if there is a good solution that doesn't take a lot of time you would/will do it

as a little sidenote: This component was migrated over from svelte(kit) and it worked there so browser seems to be fine with it?

@ryansolid
Copy link
Member

ryansolid commented Sep 10, 2024

Svelte 4 wouldn't have this issue as it creates each element separately. Svelte 5 will as they've taken on our more performant approach with template cloning. Although I admit I would still expect this to mess with hydration.

The browser doesn't error. It works except the HTML parser assumes you made a mistake and silently tries to fix it without notifying you. The end result is what get from it is different than the string you pass to it. Of course there can always differences here, when you consider encoding and quotes etc.. But in these cases when it finds invalid HTML it just re-orders stuff how it sees fit. It will hoist elements out of parents. It will put other elements inside of other elements unexpectedly and so on.

The problem is our code that attaches reactivity assumes the template is the one that we passed to the browser. So we don't know the error at runtime until we try to walk to some element's firstChild or nextSibling and it isn't there. At which point we have no idea the why as the code is just walking elements. The only place to really catch this is ahead of time or when cloning templates. But we've missed stuff for false positives/negatives with both approaches. The desire to optimize template compilation is when we moved to completely compile time so I think we can solve this there but we should come up with a systematic approach for this sort of pattern. Like we have patterns around direct descendants we could also add patterns for within a single template.

@ryansolid
Copy link
Member

We've added better detection within a single template in Solid 1.9. I don't think we can reasonably do much better than that.

Commit: 2a3a198

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

No branches or pull requests

2 participants