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

bind:valueAsDate #3527

Closed
wants to merge 4 commits into from
Closed

bind:valueAsDate #3527

wants to merge 4 commits into from

Conversation

Rich-Harris
Copy link
Member

This is a branch off, and alternative to, #3494. Out of respect for semver I think we have to treat this as a new binding, rather than changing the behaviour of the existing bind:value. In v4 we can revisit this area — I'll open a separate issue for that.

JSDOM doesn't handle this case, so the tests are incomplete pending jsdom/jsdom#2658.

Marked as draft because it doesn't yet handle the SSR case.

colincasey and others added 4 commits September 2, 2019 21:27
* (breaking change) changes the current `bind:value` behavior that returns a date string of `yyyy-mm-dd` so that it will return a `Date` value instead.
@Rich-Harris Rich-Harris marked this pull request as ready for review September 7, 2019 18:32
@Rich-Harris
Copy link
Member Author

Man, SSR was a rabbit hole, especially <input type="week">. Used https://svelte.dev/repl/a8e097072d874579a24e83e59967a30e?version=3.9.2 to test the implementation of week_input_value.

@codecov-io
Copy link

codecov-io commented Sep 7, 2019

Codecov Report

Merging #3527 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3527   +/-   ##
=======================================
  Coverage   50.25%   50.25%           
=======================================
  Files           1        1           
  Lines         197      197           
=======================================
  Hits           99       99           
  Misses         98       98

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9120f5a...8432950. Read the comment docs.

@Conduitry
Copy link
Member

I just had an idea of the syntax bind:value|date={foo} which might be more elegant.

Maybe we could also add a syntax for binding a numeric value as a string (bind:value|string={foo}?). It's unfortunate that the default would be inconsistent between numbers and dates, but correcting that (one way or the other) would have to wait for v4.

@msfragala
Copy link

@Conduitry I think that syntax makes a lot of sense. It could be a good precedent for bind:value|integer and bind:value|float, which could be useful since converting input values to numbers isn't too uncommon

@Rich-Harris
Copy link
Member Author

Yeah, I like the bind:value|date idea, since it offers a sensible route to having bind:value do the right thing for each input type in v4.

@Conduitry
Copy link
Member

do the right thing for each input type

By this do you mean that the default is always to bind as a string, and you use a |number, |date, etc. if you want something else? I think that also makes more sense than automatically binding type=number inputs as numbers.

@Rich-Harris
Copy link
Member Author

The opposite, actually — I'd like to continue using bind:value with number and range inputs without having to think about coercion. I don't think I've ever encountered a situation where I cared about the string value of a numeric input.

If we went the other way, always treating value as a string, then I'd argue for separate valueAsNumber and valueAsDate bindings rather than modifiers — simpler to implement and understand. I'm envisaging the modifiers as a way to opt back in to using the string value for the niche cases where that's desirable.

@Conduitry
Copy link
Member

Conduitry commented Nov 15, 2019

My understanding for what happened with type='date' was that we defaulted to just treating this as a normal type='text' input insofar as what events we attached and how we handled the value property - and we later came to realize that it would be convenient to actually provide the Date objects directly. If, later down the line, browsers introduce other types of inputs (or as other ones become popular) and we decide that we probably would want to bind that as a non-text value, we'll be stuck - since doing that would be a breaking change. This was my reasoning for wanting to always default to a string (which should always be safe), and to allow opt-ins to type-specific conversions, which we can gradually add over time without them being breaking changes.

I don't have a strong opinion about the syntax, but - unless I'm misunderstanding something - it seems more future proof to default to strings.

@Rich-Harris
Copy link
Member Author

Or — galaxy brain — we could treat the new input type's behaviour as a bug, and fix it in a patch.

Realistically though, we'd get months of warning about any new input type, and the semantics would be understood long before it was viable for use in production, so as long as Svelte was still being maintained if and when that happened we should be able to get ahead of it and avoid any breaking changes.

If it happened and Svelte wasn't still being developed, then we wouldn't need to worry about breaking changes anyway.

@Conduitry
Copy link
Member

Fair, but it does mean that we'd have to catch now (or at v4, or whatever) all of the existing ones that we're likely to want to handle specially. Are there other datepicker-y things besides type='date'? I think mobile browsers implement some time pickers too - and there might be separate ones for local time, I don't remember. Is there anything else we'd want to handle specially?

If you're confident that we can make a number of correct decisions all at once, then I agree that defaulting to conversion is probably more convenient for more people - but it just seems risky to me.

@msfragala
Copy link

Looking at the full list of input types on MDN, I only see two others that would make sense to handle. There's datetime-local like you mentioned, which would just be a date object. And then file, where I imagine the value would be converted to a FileList object (basically just binding to input.files instead of input.value

@Rich-Harris
Copy link
Member Author

That was my conclusion also — I think we're covered. There are some other inputs that expose a valueAsDate (and also a valueAsNumber, which is just the date.getTime()), and these would be covered by the change in v4. The other inputs with different UI (e.g. tel) are really just text inputs.

File inputs are already handled: https://svelte.dev/repl/387bf1bbbd134d0193df33ddf712fd40?version=3.14.1

I'll close this and #3494 and open a new issue for the modifiers.

@Rich-Harris Rich-Harris mentioned this pull request Nov 16, 2019
@benmccann benmccann deleted the gh-3399 branch August 22, 2023 22:05
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.

5 participants