web
web copied to clipboard
dart-lang
Add typings for JSDate
- Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.
- [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:
- See our contributor guide for general expectations for PRs.
- Larger or significant changes should be discussed in an issue before creating a PR.
- Contributions to our repos should follow the Dart style guide and use
dart format. - Most changes should add an entry to the changelog and may need to rev the pubspec package version.
- Changes to packages require corresponding tests.
Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.
Do you have an example of tests for thin wrapper declarations like this?
Since we're still defining the exact shape and conventions of this package, I'd like to make a bit of an impassioned plea that this not match web in hewing closely to the JS names and API shapes. For web itself, that design philosophy makes a lot of sense: the APIs are almost entirely autogenerated from interface definitions, so it would be onerous to manually make them match Dart conventions and have to keep those changes up to date. In addition, WebIDL already has a strong precedent of being implemented with few variations across multiple languages.
But js_interop has a different context which I think motivates a different design. Everything we expose here will be human-authored anyway, so the cost of a human hand making the design feel Dart-native is lower (and in fact I've already done most of this design work in my js_core repo). In fact, some of the APIs we intend to expose here (like JSRrecord) will necessarily be Dart-style, so there's a consistency case to be made for matching that across the package.
More importantly, though, these APIs are going to be foundational to the programming environment and commonly-used by authors doing any more than cursory Dart/JS interaction. Making them ergonomic to Dart authors and Dart conventions has an outsized benefit relative to the web APIs l, most of which will each individually be used less. That's particularly true for the APIs I'm planning to add to dart:js_interop, which this package should certainly match in style.
Along that line, I'd also make the case that dart:js_interop already leans towards using Dart conventions rather than JS. JSFunction has methods named callAsMethod and callAsConstructor when the JS equivalents are named apply() and construct(). While JSAny? does have an all-lowercase instanceof method, that naming is already inconsistent: it also defines instanceOfEquals.
The final case I'll make for using a more Dart-native style is for the ecosystem as a whole. Since their inception with collection, core-extension packages in the Dart ecosystem have served as the blueprints for what well-designed, idiomatic Dart APIs look like. We're in the very early days of the new (and likely final) shape of JS interop in Dart, and the APIs we release here and in my upcoming additions to the SDK will go a long way towards defining what a good JS API wrapper should look like everywhere. One of the things that makes the new JS interop system so exciting is that it makes it possible to use JS in a Dart-y way with near zero overhead. For a simple data type like Date this just looks like a few functions with different capitalization, but once we get into more complex and interesting APIs the usability difference will become profound. Let's not let the potential of this new system go to waste.
re: tests, we do this for some of the dart:js_interop APIs by just testing that a method gives the expected value with the expected type e.g. https://github.com/dart-lang/sdk/blob/59905c43f1a0394394ad5545ee439bcba63dea55/tests/lib/js/static_interop_test/js_types_test.dart#L237. Nothing fancy, just making sure there are no spelling or type issues. Ideally, we'd have these for all of the members in package:web, but that's impractical and because it's generated, it's less of an issue.
I think there's a valid argument to say this package is a better candidate to be Dart-y than package:web. The latter is generated and making that Dart-y would revive what made dart:html hard to maintain. Unlike package:web too, I expect the spec for the types in this package to be much more stable.
I'd still argue there are some benefits to having a model that translates directly to JS. I want and expect users to look for JS documentation when dealing with interop types, and I think it's easier for users to directly write those method/member names and have them exist/autocomplete properly. It's a (small) tax to have to look at the API and figure out how to make the equivalent JS call. I bet this tax goes up as the APIs get more complex and we get more opinionated.
Along that line, I'd also make the case that dart:js_interop already leans towards using Dart conventions rather than JS. JSFunction has methods named callAsMethod and callAsConstructor when the JS equivalents are named apply() and construct(). While JSAny? does have an all-lowercase instanceof method, that naming is already inconsistent: it also defines instanceOfEquals.
The instances where we do this are usually out of necessity. callAsFunction is actually call but we didn't want to expose that as call because then you can call it with () syntax, which sounds great, until you realize call takes in a this parameter as the first parameter. I suppose we could have a patched invoke method that actually invokes the function instead of .call, and I believe there's a request somewhere for that. callAsConstructor also doesn't use Reflect - it's a patched member that invokes the function with new. instanceOfString is more of a helper and we provide instanceof still (typeofEquals should really have an equivalent typeof member users can call). There are a few other odd cases in dart:js_interop like JSArray.add (instead of push) that have some specific context behind them.
All of this being said, helpers are great. That's partially why we have some of the "helpers" in package:web as well as conversion methods. But if we're only going to provide the "helper" version, it does mean we're going to need to be opinionated on how to expose JS interop and stay consistent with that opinion. More specifically to this PR, a third option is providing both e.g. the interop method that the more Dart-y APIs call and the Dart-y APIs themselves. That may be overkill though.
The final case I'll make for using a more Dart-native style is for the ecosystem as a whole. Since their inception with collection, core-extension packages in the Dart ecosystem have served as the blueprints for what well-designed, idiomatic Dart APIs look like. We're in the very early days of the new (and likely final) shape of JS interop in Dart, and the APIs we release here and in my upcoming additions to the SDK will go a long way towards defining what a good JS API wrapper should look like everywhere.
I think I feel slightly different about interop-related packages because they're inherently dealing with another language. This argument makes sense for any helpers we add, but they shouldn't be complete replacements.
For a simple data type like Date this just looks like a few functions with different capitalization, but once we get into more complex and interesting APIs the usability difference will become profound.
My above comment is relevant here: I expect the tax of trying to match the JS call to the Dart API we expose will go up as we look at the more complex cases but we can talk about those when we get there.
I think the tax of having to remember not to use Dart naming conventions when writing Dart code is likely to be higher than the tax of not exactly matching the JS API names, especially since no matter what we can't match JS names exactly—as you point out, there will always be cases like call that can't be written in Dart for technical reasons. Authors will never be able to rely on names being JS-style without caveats, but they would be able to rely on them being Dart-style without caveats.
I feel more strongly about this for places where the JS names outright violate Dart naming conventions (like Date.utc) than I do for cases where I'm reusing a JS name to do something slightly different (like Date.now). Maybe we can compromise on only doing the former and renaming the latter to Date.nowAsDate and so on?
Authors will never be able to rely on names being JS-style without caveats, but they would be able to rely on them being Dart-style without caveats.
I agree that there will always be exceptions due to the nature of keywords/Dart semantics, and I can see the argument that e.g. reaching for the all-uppercase version is less intuitive than the camel-cased version, but I think a strict adherence to Dart style for interop, e.g. getHours and setHours being moved to a getter-setter pair, can be less intuitive and requires reading the API documentation. Maybe my mental model is wrong, but I assume interop users start from wanting to call a JS member/functionality and work their way backwards.
I feel more strongly about this for places where the JS names outright violate Dart naming conventions (like Date.utc) than I do for cases where I'm reusing a JS name to do something slightly different (like Date.now). Maybe we can compromise on only doing the former and renaming the latter to Date.nowAsDate and so on?
Sounds good, I don't have a strong opinion on capitalization. utc should be barely less findable than UTC.
I've added tests, updated member names, and added a new Date.utcDate constructor (and renamed Date() to Date.localDate() to make the distinction explicit). PTAL.
Maybe my mental model is wrong, but I assume interop users start from wanting to call a JS member/functionality and work their way backwards.
I wonder if this is the core disconnect here. When I'm writing interop code, the biggest point of friction is usually context switching. This is especially true when I'm dealing with APIs that are weird shapes for historical reasons—remembering that DateTime.now() gets me the current date in Dart but Date.now() gets an int in JS is a speedbump, remembering that some JS methods inherited Java's get... and set... before the language added real properties is a speedbump, and so on. It's even worse if I'm writing one language and dealing with quirks from a different one.
The thing that eases this pain is API consistency, which is why I'm advocating for it so fervently. If I can know that when I'm writing Dart code, APIs will be Dart-shaped, historical artifacts will be smoothed out, and names will follow Dart conventions, my life becomes much easier.
By contrast, having the APIs match precisely across languages is relatively low value to me (as long as the correspondence is documented). I rarely access APIs entirely by memory, even when I'm working in a single language that I'm familiar with (even when I'm using APIs I personally created!). I rely on documentation, either through the web or through my editor, to remind me the precise names or arguments of whatever I'm interacting with. If I'm writing JS interop code in Dart, I'd be looking those up in the Dart documentation anyway, so having them be a little more different from the JS in exchange for them being a little more ergonomic at every use-site is a tradeoff that's well worth its cost to me.
I wonder if this is the core disconnect here.
Probably. :D I also think this will differ between folks who will use these types regularly versus folks who use these once.
If I'm writing JS interop code in Dart, I'd be looking those up in the Dart documentation anyway, so having them be a little more different from the JS in exchange for them being a little more ergonomic at every use-site is a tradeoff that's well worth its cost to me.
I suppose one benefit types in this package will have over package:web is they'll be more intuitive and require less reason to go look for documentation if the name makes it fairly obvious.
But okay, so that we make progress, let's go with the strategy that you propose and make things more Dart-y and ergonomic where it makes sense. In this PR that means capitalization modifications and moving obvious methods to getters/setters.
I expect us to do a pre-release, and we can get more input (if any) from other users on the conventions then before releasing a 1.0.0.
I expect us to do a pre-release, and we can get more input (if any) from other users on the conventions then before releasing a 1.0.0.
Sounds good, collecting feedback from users at large is definitely the most robust way to answer this question :slightly_smiling_face:.
I expect us to do a pre-release, and we can get more input (if any) from other users on the conventions then before releasing a 1.0.0.
Sounds good, collecting feedback from users at large is definitely the most robust way to answer this question 🙂.
JS: ugh Testing: yay!