Ensure that Temporal prototypes aren't writable #108
Conversation
| @@ -8,4 +8,29 @@ | |||
| import * as Temporal from './temporal'; | |||
| import * as Intl from './intl'; | |||
| import { toTemporalInstant } from './legacydate'; | |||
|
|
|||
| // Work around https://github.com/babel/babel/issues/2025. | |||
There was a problem hiding this comment.
This has already been fixed in Babel - maybe we should just update our build once they release instead?
There was a problem hiding this comment.
Do we know their release schedule? 7.16.4 was almost a month ago according to GitHub, so if it's monthly and we only have to wait a few days then that seems like a good idea.
There was a problem hiding this comment.
I actually think it's somewhat unrelated to Babel's schedule, because (unless I'm missing something) anyone who uses babel while bundling their app will be vulnerable. So even if our bundling is OK with the new Babel, we'll still want to protect users who are using an older Babel version from being able to build code that replaces the prototype (which will break when they upgrade to the latest Babel).
BTW, we still need to merge the changes to Intl.DateTimeFormat prototype regardless of Babel.
There was a problem hiding this comment.
If we required at least 7.16.5 wouldn't users' babel update when they updated @js-temporal/polyfill to 0.3.0, as long as the versions were compatible?
There was a problem hiding this comment.
Maybe I'm mis-reading this, but it seems that devDependencies (which is what our dep on babel is) aren't installed transitively, so we wouldn't be forcing downstream users to update their babel versions.
That being said, I wonder if there's a way to "suggest" that update to users to encourage they update.... maybe an (ab)use of peerDependencies can do this?
There was a problem hiding this comment.
Yep, my understanding of build-time dependencies is that they're not enforced on the main app.
Regardless, I'm not sure it's worth the bother. We understand the problem and have a simple workaround that seems to work (and is a no-op at runtime if it's been transpiled with a fixed Babel). So I'm inclined to just declare success and move on. What do you guys think?
There was a problem hiding this comment.
That sounds reasonable - let's just merge this and move on :)
lib/intl.ts
Outdated
| @@ -170,12 +170,12 @@ function DateTimeFormatImpl( | |||
| } | |||
|
|
|||
| Object.defineProperty(DateTimeFormatImpl, 'name', { | |||
| writable: true, | |||
| writable: false, | |||
There was a problem hiding this comment.
We don't run our polyfill against the whole test262 suite (and that's probably not a good idea), but from browsing it looks like this property is actually intended to be configurable: https://github.com/tc39/test262/blob/main/test/intl402/DateTimeFormat/name.js
There was a problem hiding this comment.
Oh! I'll revert. I assumed this was a mistake. Serves me right for not checking the spec first.
| @@ -8,4 +8,29 @@ | |||
| import * as Temporal from './temporal'; | |||
| import * as Intl from './intl'; | |||
| import { toTemporalInstant } from './legacydate'; | |||
|
|
|||
| // Work around https://github.com/babel/babel/issues/2025. | |||
There was a problem hiding this comment.
Do we know their release schedule? 7.16.4 was almost a month ago according to GitHub, so if it's monthly and we only have to wait a few days then that seems like a good idea.
justingrant
force-pushed
the
no-writeable-prototypes-port-1974
branch
from
381f234 to
fd4c19c
Compare
lib/index.ts
Outdated
| Temporal.PlainDateTime, | ||
| Temporal.Duration, | ||
| Temporal.PlainMonthDay, | ||
| // Temporal.Now, |
There was a problem hiding this comment.
Either remove, or add a comment explaining why Now isn't also not-writeable?
There was a problem hiding this comment.
I think it's because Now doesn't have its own prototype, it's a plain object, but agreed a comment would be good
There was a problem hiding this comment.
Yep, that's why it's omitted. I'll add a comment and merge.
There was a problem hiding this comment.
And I'll do the same thing for the proposal-temporal equivalent PR. @ptomato could I get a review on that one? tc39/proposal-temporal#1974
justingrant
force-pushed
the
no-writeable-prototypes-port-1974
branch
from
fd4c19c to
6f3ca25
Compare
Port of tc39/proposal-temporal#1974