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

dataset optimization does not respect null indicating to remove the attribute #3337

Closed
Conduitry opened this issue Aug 3, 2019 · 3 comments · Fixed by #3346
Closed

dataset optimization does not respect null indicating to remove the attribute #3337

Conduitry opened this issue Aug 3, 2019 · 3 comments · Fixed by #3346
Assignees
Labels

Comments

@Conduitry
Copy link
Member

Describe the bug
data-foo={bar} should, like other attribute settings, remove the attribute when setting it to a value of null or undefined. The optimization (used unless legacy is enabled) of instead assigning to the dataset object prevents this.

To Reproduce

<script>
	let name = 'world';
	let toggle = false;
</script>

<input type='checkbox' bind:checked={toggle}>

<h1 data-foo={toggle ? name : null}>Hello {name}!</h1>

Expected behavior
The data-foo attribute should be not present when the checkbox is unchecked, not the string 'null'.

Severity
Not personally affecting me.

Additional context
Came up in regard to this question. I believe this should be the way to handle that, but it doesn't work.

@Conduitry Conduitry added the bug label Aug 3, 2019
@Conduitry
Copy link
Member Author

I'm not sure the extent that #858 was intended as a speed optimization vs a size optimization. The simplest solution would be to remove that optimization (to bring this in line with the other recent move to do almost everything with attributes instead of properties), but I don't know what that would lose.

@Rich-Harris
Copy link
Member

If it was intended as a speed optimisation it was misguided:

Screen Shot 2019-08-03 at 13 31 06

I think it was a size thing more than anything. Dropping it seems like the straightforward option. With that change, legacy would only be used for readonly input.type properties and erroring on once/passive event modifiers.

@Conduitry Conduitry self-assigned this Aug 3, 2019
@Conduitry
Copy link
Member Author

Sounds good - I'll prepare a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants