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

Loose comparison when early-outing from setting attributes #1595

Merged
merged 1 commit into from Feb 18, 2017

Conversation

Projects
None yet
5 participants
@tivac
Copy link
Member

tivac commented Feb 6, 2017

This is a rough attempt at a fix for #1593, with zero consideration given to potential side-effects. Just something to get started discussing more than anything.

@tivac tivac changed the title Loose comparison when early-outing from setting attribtes Loose comparison when early-outing from setting attributes Feb 6, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Feb 8, 2017

IIRC a similar workaround had to be done in 0.2.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Feb 8, 2017

Indeed, there was (top and bottom of the selected range).

There are eslint disable comments around the code because the version of ESLint used in the v0.2 branch was automatically "fixing" them to strict equals. I suppose that doesn't apply here.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Feb 9, 2017

@pygy That rule is currently disabled, so you don't have to disable it.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Feb 9, 2017

I'm convinced. When I have some time I'll add some tests and merge this.

@lhorie lhorie merged commit 732ddf0 into MithrilJS:next Feb 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pygy

This comment has been minimized.

Copy link
Member

pygy commented Feb 18, 2017

@isiahmeadows We could enable it now, ESLint stopped "auto-correcting" loose equality when they released v2 (Mithril v0.2 is on ESLintv1.7, Mithril v1 is on ESLint v2).

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Feb 20, 2017

ESLint's latest is v3.16...I think we should upgrade at some point.

@tivac tivac deleted the tivac:issues-1593 branch Feb 21, 2017

@lhorie

This comment has been minimized.

Copy link
Member

lhorie commented Feb 23, 2017

ESLint is at latest now

@boblehest

This comment has been minimized.

Copy link

boblehest commented May 25, 2017

For the record, this breaks m("option", {value: 0}), because 0 == "" (thus the value is ignored.)

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 25, 2017

@boblehest Could you provide a jsbin that demonstrates the issue?

@boblehest

This comment has been minimized.

Copy link

boblehest commented May 26, 2017

@pygy Sure thing: http://jsbin.com/gekipedipa/edit?js,output

The HTML output looks like this:

<select>
    <option value="-1">Minus one</option>
    <option>Zero</option>
    <option value="1">One</option>
</select>

The fix is of course just to stringify your values, which makes sense to do since the value is stored as a string anyway. I'm just mentioning it because this change broke my old code, and because I think using loose comparison is wrong 99% of the time.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 26, 2017

Wow, thanks. Loose comparison looked like the correct solution here, but it was assuming a slightly stricter ==.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 27, 2017

So, new PR?

It's odd that we have browser inconsistency here, though.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 27, 2017

I'm working on it... What's inconsistent? I see the same result in Firefox, Chrome, Safari, Edge14 and IE11.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 27, 2017

@pygy I was referring to the original apparent browser bug being fixed by this PR.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 27, 2017

Ok, yes, Firefox preserves the cursor position in that case, WebKit derivatives don't...

I'm currently adding tests for this PR before I start fixing the issue raised by @boblehest. The mocks had to be improved (more faithful attributes implementation, optional spies on setters, with a hatch on elements to retrieve them).

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 27, 2017

@pygy Idea: maybe cast the value to a string before comparing? That might help alleviate this problem while still avoiding a sloppy check.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 27, 2017

@isiahmeadows that was the idea, yes... There's a corner case with symbols too (using a symbol as attribute causes a TypeError: Cannot convert a Symbol value to a string, as if one was trying to do the conversion with "" + symbol... I guess we can use value = value === null ? "" : "" + value directly, since having a Symbol will cause the same error later on anyway...

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 28, 2017

@pygy You can coerce Symbols as well via String(sym), but in general, it's likely erroneous. As for the coercion, I'd do == null rather than === null, to also capture undefined.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 28, 2017

@isiahmeadows yes, I know about String(symbol), but do we want to do that? The DOM property setters and setAttribute() do '' + symbol. Strict equality is intentional, to mimic the DOM APIs where passing undefined sets "undefined", whereas null becomes "", which is IMO a wart, but changing that would break B/W compat (and we'd need to do it for every attribute for consistency, not just input/option/select.value... I don't know if there aren't properties with a different behavior. We'd have to special case custom elements, for sure).

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 28, 2017

Actually, the bin provided by @boblehest works fine since #1828 (on first render, I thought I was going crazy since I couldn't repro the issue in the test suite).

Updating the tree would still break when switching between "" and 0.

pygy added a commit to pygy/mithril.js that referenced this pull request May 28, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request May 28, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request May 28, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented May 29, 2017

@pygy

[...] but do we want to do that?

Sorry if it wasn't clear enough, but I tried to imply a weak "no", more like a -0.5.

pygy added a commit to pygy/mithril.js that referenced this pull request May 29, 2017

pygy added a commit to pygy/mithril.js that referenced this pull request May 29, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented May 29, 2017

#1862 should fix this

isiahmeadows pushed a commit that referenced this pull request May 31, 2017

Isiah Meadows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment