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

Clarify null and undefined serialization binding behavior in docs #204

Closed
klebba opened this issue Nov 12, 2024 · 6 comments · Fixed by #206
Closed

Clarify null and undefined serialization binding behavior in docs #204

klebba opened this issue Nov 12, 2024 · 6 comments · Fixed by #206

Comments

@klebba
Copy link
Collaborator

klebba commented Nov 12, 2024

There are some nuances regarding binding null and undefined binding behavior in XElement — philosophically we want to leverage (and not augment) the browser's behavior.

Add detailed info here: https://github.com/Netflix/x-element/blob/main/doc/TEMPLATES.md

Maybe update the philosophy: "Please do not make me learn anything proprietary about the framework unless it is absolutely necessary!"

Consider:

// uses element.textContent = binding
html`<div>${null}</div>`
// -> <div></div>
html`<div>${undefined}</div>`
// -> <div></div>


// uses element.innerHTML = binding (unsafe!)
html`<div>${unsafeHTML(null)}</div>`
// -> <div></div>
html`<div>${unsafeHTML(undefined)}</div>`
// -> <div>undefined</div>


// uses element.setAttribute('name', binding)
html`<div test-null="${null}"></div>`
// -> <div test-null="null"></div>
html`<div test-undefined="${undefined}"></div>`
// -> <div test-undefined="undefined"></div>


// uses element.setAttribute('name', binding)
html`<div test-null="${null ?? ''}"></div>`
// -> <div test-null></div>
html`<div test-undefined="${undefined ?? ''}"></div>`
// -> <div test-undefined></div>


// uses binding ? element.setAttribute('name', '') : element.removeAttribute('name') (boolean attribute behavior)
html`<div ?test-null="${null}"></div>`
// -> <div></div>
html`<div ?test-undefined="${undefined}"></div>`
// -> <div></div>


// magic helper (undefined)
/*
 if (binding === undefined) {
   element.removeAttribute('name')
 } else {
   element.setAttribute('name', binding)
}
*/
html`<div test-null="${ifDefined(null)}"></div>`
// -> <div test-null="null"></div>
html`<div test-undefined="${ifDefined(undefined)}"></div>`
// -> <div></div>


// magic helper (nullish)
/*
 if (binding === undefined || binding === null) {
   element.removeAttribute('name')
 } else {
   element.setAttribute('name', binding)
}
*/
html`<div test-null="${nullish(null)}"></div>`
// -> <div></div>
html`<div test-undefined="${nullish(undefined)}"></div>`
// -> <div></div>


// These cases throw an error
html`<div>${ifDefined(null)}</div>`
html`<div>${ifDefined(undefined)}</div>`
html`<div>${nullish(null)}</div>`
html`<div>${nullish(undefined)}</div>`
html`<div ?test-null="${ifDefined(null)}"></div>`
html`<div ?test-undefined="${ifDefined(undefined)}"></div>`
html`<div ?test-null="${nullish(null)}"></div>`
html`<div ?test-undefined="${nullish(undefined)}"></div>`

Perhaps the structure of the docs ought to be like "How do I ..."

  • bind string content to the DOM
  • bind real markup to the DOM
  • bind string values to the DOM (attribute=value)
  • bind boolean values to the DOM (presence of attribute name)
  • only bind an attribute to the DOM when the value is not nullish (for important cases like <a href="">)
@klebba klebba changed the title Clarify null and undefined binding behavior in docs Clarify null and undefined serialization binding behavior in docs Nov 12, 2024
@klebba
Copy link
Collaborator Author

klebba commented Nov 12, 2024

Andrew sayz:

ya gotta be careful not to create a text node with a null or undefined binding rather than create an empty text node and then bind null or undefined to its text content (which might delete that node but that's ok because it is native behavior)

@klebba
Copy link
Collaborator Author

klebba commented Nov 12, 2024

Casey sayz:

we really oughtta nuke ifDefined and nullish directives from the core; these can be easily achieved through modern ternary syntax

But Casey is wrong :( because removeAttribute would never be called and that is really the purpose of these directives. Further nullish is behaving like ifNotNullish which is certainly clumsy, and not congruent with ifDefined which is borrowed from Lit for "migratory" reasons

@klebba
Copy link
Collaborator Author

klebba commented Nov 12, 2024

Note these two are exactly equivalent for the purpose of CSS selectors and JS introspection:

<div foo=""></div>
<div foo></div>

@klebba
Copy link
Collaborator Author

klebba commented Nov 12, 2024

Consider reworking ifDefined and nullish for v2 as a breaking change. Possible ideas:

  • <div ~test="${foo}"></div> -> <div></div> when foo is nullish
  • <div ??test="${foo}"></div> -> <div></div> when foo is nullish
  • <div test="${when(foo)}"></div> -> <div></div> when foo is nullish

@klebba
Copy link
Collaborator Author

klebba commented Nov 12, 2024

Directive overhaul is planned in v2:

  • Agree to deprecate / undocument nullish as it is on the path to revision and not generally adopted.
  • Agree to deprecate repeat and ifDefined as well.
  • Consider deprecating live if we can show how to achieve the same outcome using a documented example (e.g. maybe add doc/Forms.md or doc/Directives.md)
  • Delete use of createTextNode with a binding - instead we will create the node and then bind via textContent in order to remove our inner nullish test which is adding a proprietary behavior/opinion that is subtle and unexpected
  • Introduce the ??attr="${binding}" syntax which will replace ifDefined and nullish

theengineear added a commit that referenced this issue Nov 14, 2024
Changes:
* Adding `??attr` binding syntax.
* Chaning `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 14, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 14, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 14, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 14, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 15, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
@theengineear
Copy link
Collaborator

Related to your ask for examples / documentation on live — here is an issue ticket which discusses the pros / cons: #208.

theengineear added a commit that referenced this issue Nov 15, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 15, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 15, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 16, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 16, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
theengineear added a commit that referenced this issue Nov 16, 2024
Changes:
* Adding `??attr` binding syntax.
* Changing `ifDefined` to delete attr on `null` as well as `undefined`.
* Deprecating `ifDefined` / `nullish` in favor of `??attr` binding.
* Deprecating `repeat` in favor of `map`.
* Updating documentation.

Closes #204.
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 a pull request may close this issue.

2 participants