Changes to keep aligned with discussion #76
Conversation
|
Bikeshed: We don't have anything in the standard library with the |
|
@littledan I agree, but That's why I'm asking for better names! Currently I have both Oh and making it |
polyfill/lib/civildate.mjs
Outdated
| const { year, month, day } = this; | ||
| return `${spad(year, 4)}-${pad(month, 2)}-${pad(day, 2)}`; | ||
| } | ||
| toString() { | ||
| return `[object CivilDate ${this.asString()}]`; |
There was a problem hiding this comment.
This is a very strange result that I think would be a bad idea. If it's going to be in the [object X] form, it should follow that convention.
Rather than defining a toString, it would probably want get [Symbol.toStringTag]() { return CivilDate ${this.asString(); } - but I'd still think this is way less useful than returning the previous toString implementation.
polyfill/lib/civildate.mjs
Outdated
| @@ -13,25 +13,40 @@ export class CivilDate { | |||
| const { year, month, day } = plus({}, { years, months, days }); | |||
| this[DATA] = { year, month, day }; | |||
| } | |||
| get [Symbol.toStringTag] () { return 'CivilDate'; } | |||
There was a problem hiding this comment.
these shouldn’t be getters (unless they’re going to brand check before returning the string, which I’d be super in favor of)
There was a problem hiding this comment.
Why? What?
Since class public fields are not a thing yet, ...
Also what do you mean by "brand check"? Check that it's an instance of ?
Explain, I'm confused.
There was a problem hiding this comment.
Since class fields aren’t a thing yet, it simply can’t live inside the class body. You’d want to do CivilDate.prototype[Symbol.toStringTag] = ‘CivilDate’; after the class definition.
A brand check would be if it threw, or returned null, when the receiver lacked the right internal slot - in other words, if you did new CivilDate()[Symbol.toStringTag] you’d get ’CivilDate’, but if you did CivilDate.prototype.toStringTag it would throw.
|
I still think the polyfill code here should be removed from this repo and moved to a separate one. |
|
@ljharb right now having them together is very useful, since it's easier to develop the spec and the polyfill concurrently, one bouncing off the other. At the same time I'm with you as regards to making it easier to publish to npm. We should also probably schedule another live call, with everyone there, and have the discussion verbally to hash it out. |
|
No, don't make Symbol.toStringTag a brand-checked getter, that's inconsistent with the rest of JavaScript. Just do what evertyhing else does... |
|
obsolete due to #88 |
.now()as per new Instant() or Instant.now()? #73.valueOf()with.valuein line with Consider removing toString and valueOf methods #74.toString()gives [object Type ISO-String] in line with Consider removing toString and valueOf methods #74.asString()gives ISO-StringI named the method
.asString()for now, but that should be name-checked (open for suggestions). I really don't liketoISOString()because that suggests generi ISO. However the string produced has quite detailed specification in order to clarifyfromString(). (Maybe I'm just too hestitant, but it just gives me heebe-jeebies)