Skip to content

Fix how x-html handles undefined #4555

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

willrowe
Copy link
Contributor

@willrowe willrowe commented Feb 25, 2025

If x-html resolves to an empty string or null, then innerHTML is set to an empty string. However, if it resolves to undefined, then innerHTML is set to a string with value of undefined, which is displayed to the user.

Since null already acts as expected and displays nothing to the user, this PR updates x-html to leave the current innerHTML as-is if the value resolves to undefined. This way nothing unexpected is displayed to the user and also makes it possible to include fallback HTML on page load that will be displayed until the value resolves to something other than undefined.

- If you want to remove all HTML and show nothing then x-html should resolve to an empty string or null.
- To leave the initially loaded or current HTML displayed then x-html should resolve to undefined.

This PR has changed focus, see this comment

@ekwoka
Copy link
Contributor

ekwoka commented Feb 26, 2025

I feel like the expectation might still be that it would clear it out... 🤔

But this seems sensible.

Copy link
Contributor

@ekwoka ekwoka left a comment

Choose a reason for hiding this comment

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

Can you update the documentation for x-html to describe these behaviors?

@willrowe
Copy link
Contributor Author

willrowe commented Feb 26, 2025

@ekwoka

I feel like the expectation might still be that it would clear it out

I went back and forth on it, so I'm not completely sold on this. I think there should be some way to have the original markup remain displayed while the value of the expression is not set, perhaps a modifier would be better?

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 26, 2025

You can easily achieve it with the ?? operator if you are unsure when the data will be available

<div x-data="{foo: undefined}" x-init="setTimeout(() => {foo = 'loaded'}, 5000)">
  <p x-html="foo"><b>loading</b></p>
  <p x-html="foo ?? $el.innerHTML"><b>loading</b></p>
</div>

@willrowe
Copy link
Contributor Author

@SimoTod I am aware, that is exactly what I'm doing now to work around it.

@ekwoka
Copy link
Contributor

ekwoka commented Feb 27, 2025

@SimoTod Technically speaking, that would destroy and recreate the contained dom tree, which MAY be a problem.

@SimoTod
Copy link
Collaborator

SimoTod commented Feb 27, 2025

Yeah but it's on page load so it's unlikely to have any meaningful state. After that point, the variable shouldn't really be undefined unless the app follows a weird design.

This is obviously not relevant to the PR itself, it was just an interim workaround for that use case.

@willrowe
Copy link
Contributor Author

I think I'll change this PR to be a simple fix for the undefined string being displayed. I tested out x-text, which calls textContent under the hood. textContent coerces both null and undefined to an empty string unlike innerHTML which, as @ekwoka mentioned, really is what most people would expect.

I will explore adding an ignore-nullish modifier in another PR.

@ekwoka
Copy link
Contributor

ekwoka commented Feb 27, 2025

Could you update the docs to cover this?

- `innerHTML` does not handle `undefined` values as expected. It will display the word `undefined` instead of using an empty string like `textContent` does.
@willrowe willrowe force-pushed the feature/retain-html-on-undefined branch from e36d6db to 8d0a9dc Compare February 27, 2025 16:14
@willrowe willrowe changed the title Do not change HTML if x-html is set to undefined Fix how x-html handles undefined Feb 27, 2025
@willrowe
Copy link
Contributor Author

This PR has been updated to fix an inconsistency in how innerHTML handles nullish values. A value of undefined now sets innerHTML to an empty string to match how textContent functions.

@ekwoka since this is now a bug fix, what needs to be updated in the docs?

@ekwoka
Copy link
Contributor

ekwoka commented Feb 27, 2025

Okay, with that change, I don't think it would be needed.

With it preserving the original content it would be valuable to document it.

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