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

[WIP] Remove an undefined attribute instead of setting it to "undefined" #1668

Closed
wants to merge 2 commits into from

Conversation

kaisermann
Copy link
Member

Related to #1434

@kaisermann
Copy link
Member Author

Does it make sense to use == null instead of === undefined to catch null attributes as well?

@Rich-Harris
Copy link
Member

Nice, thanks!

Hmm... good question. I think so? I suppose it's hard to imagine a situation where you'd want the string literal "null" instead.

I noticed the test has skip-ssr — would be great if we can get the same behaviour in both targets 😀

@Rich-Harris
Copy link
Member

btw to update the js tests, you can do node test/js/update.js rather than updating them manually, once you're happy that the generated code is correct

@kaisermann
Copy link
Member Author

@Rich-Harris Oh, nice to know 🤣 I've just updated them manually...

I didn't have time to mess around with svelte's ssr side but I will try to make it work!

@kaisermann kaisermann changed the title Remove an undefined attribute instead of setting it to "undefined" [WIP] Remove an undefined attribute instead of setting it to "undefined" Aug 21, 2018
@jacwright
Copy link
Contributor

@kaisermann what about property and data attributes? E.g. id attributes such as in https://svelte.technology/repl?version=2.12.0&gist=e3ec1b2f5f00eaf3a2afe8f79ce571d4

I actually did a full branch that prevented undefined from showing up in attributes and text but then ran across #659 (and didn't see your PR either until now). Not sure if we want it, if it goes too far and we want to keep pieces of it, or what.

https://github.com/sveltejs/svelte/tree/mustache-undefined

It does solve the property/data-set issue by adding methods for checking.

@Rich-Harris
Copy link
Member

Closing this in favour of #1815 (which is this + SSR support)

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