-
Notifications
You must be signed in to change notification settings - Fork 43
Add typings for JSDate #484
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
base: main
Are you sure you want to change the base?
Conversation
|
No tests? 😁 |
srujzs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for adding this Natalie!
+1 to a quick test just to make sure we have the right bindings.
| /// The [Date constructor] that returns the current date and time. | ||
| /// | ||
| /// [Date constructor]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date | ||
| external JSDate.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this might be a little confusing as intuitively, one would expect the static method Date.now to be called and not a constructor. What about JSDate.nowAsDate so that we don't need to rename now below?
| external JSDate(int year, int month, | ||
| [int? day, int? hours, int? minutes, int? seconds, int? milliseconds]); | ||
|
|
||
| /// Dee [`Date.now()`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: See
| /// | ||
| /// [Date constructor]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date | ||
| /// [from a string]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/Date#date_string | ||
| external JSDate.parse(String dateString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to now, what about parseAsDate instead so we can keep parse below?
| /// | ||
| /// [`Date.utc()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/UTC | ||
| @JS('UTC') | ||
| external static int utc(int year, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We avoid renaming to lowercase with package:web e.g. URL, maybe worth doing the same?
Same comment on toIsoString, toUtcString, and toDartUtc.
| /// [`Date.getDate()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getDate | ||
| /// [`Date.setDate()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/setDate | ||
| int get date => _getDate(); | ||
| set date(int value) => _setDate(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should keep the syntax the same as JS here by just exposing the methods instead of trying to be more Dart-y like we do in package:web.
Same comment everywhere else.
|
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 But 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 Along that line, I'd also make the case that The final case I'll make for using a more Dart-native style is for the ecosystem as a whole. Since their inception with |
|
re: tests, we do this for some of the I think there's a valid argument to say this package is a better candidate to be Dart-y than 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.
The instances where we do this are usually out of necessity. All of this being said, helpers are great. That's partially why we have some of the "helpers" in
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.
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. |
Contribution guidelines:
dart format.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.