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

Discussion for v2: make the attrs take precedence over what the selector provides #2172

Closed
pygy opened this issue Jun 1, 2018 · 6 comments
Closed
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code

Comments

@pygy
Copy link
Member

pygy commented Jun 1, 2018

Currently,

m("foo[bar=baz]", {bar: "qux"})

results in:

{tag: "foo", attrs: {bar: "baz"}}

In other words, the selector has precedence over the attrs. This kind of made sense as an optimization prior to this PR, (so that classes would end up set on className which is faster to set according to Leo). But it may be useful to let the attrs take precedence, so that a common selector string can have its attrs overruled in some circumstances.

Once we have that, however, comes the question of null-valued attrs:

As far as m.render() is concerned, an attr with a nullish value is equivalent to a lack of attribute. How should m() work in similar situations?

Should m("foo[bar=baz]", {bar: null}) produce {bar: "baz"} or {bar: null} as attrs?

The former is easier to document, the the latter is more powerful, especially In a ES6+ world, where you can do {...(condition ? {bar: null} : null)}.

Thoughts? This is related to #1773 which I had overlooked for v2.

@pygy pygy added the Type: Breaking Change For any feature request or suggestion that could reasonably break existing code label Jun 1, 2018
@pygy
Copy link
Member Author

pygy commented Jun 2, 2018

On top of the above, what should m(".foo", {class: null}) end up being?

m(".foo", {class: "bar"}) currently sets the class to "foo bar" (the className, really, but it is an implementation detail). I think that behavior should be preserved, even if the attrs otherwise take precedence.

If the attrs take precedence over the selector, though, m(".foo", {class: null}) could either not have any class, or have null treated as the empty string and have the class set to "foo".

Or maybe this whole discussion is misguided, and we should keep the current behaviour where the selector has precedence...

@barneycarroll
Copy link
Member

Non-special attributes should override their selector-defined counterparts. I can't think of a salient scenario where someone might define an attribute in the selector then have a conditionally nullish counterpart in the attrs hash, expecting it to fall back to the selector value.

Compound className definition should definitely have nullish fall through to 'nothing to add' as opposed to 'delete' though.

@pygy
Copy link
Member Author

pygy commented Jun 2, 2018

Let's try that...

Leaving this open for now, I'm not entirely convinced this is the way to go.

pygy added a commit to pygy/mithril.js that referenced this issue Jun 2, 2018
pygy added a commit to pygy/mithril.js that referenced this issue Jun 2, 2018
@futurist
Copy link
Contributor

futurist commented Jun 3, 2018

I support direction of @barneycarroll point, that mithril should do less magic, should keep simple logic in vnode level.

Or even, I'd like ignore all nullish value in attrs, that's what null comes to: nothing can do. like below:

m('', {onclick: null})

In any way it should generate same vnode to:

m('')

Strictly.

@futurist
Copy link
Contributor

futurist commented Jun 4, 2018

Where to discuss other features in V2?

@pygy
Copy link
Member Author

pygy commented Jun 4, 2018

@futurist You look for issues and PRs with the breaking change proposal label, or open a new one if something is not addressed yet.

Edit: You can also look at the change log for more things that are already in.

Regarding nullish attrs, that's how next behaves right now. The trickiest part is dealing with attrs defined on both the selector and the attrs object, but the current direction looks solid.

Agreed in general regarding the "less magic" mantra.

@pygy pygy closed this as completed in 92b22fe Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change For any feature request or suggestion that could reasonably break existing code
Projects
None yet
Development

No branches or pull requests

3 participants