Ensure that Temporal prototypes aren't writable by justingrant · Pull Request #108 · js-temporal/temporal-polyfill

justingrant · 2021-12-13T05:58:23Z

Port of tc39/proposal-temporal#1974

12wrigja · 2021-12-13T18:02:07Z

lib/index.ts

 import * as Intl from './intl';
 import { toTemporalInstant } from './legacydate';
+
+// Work around https://github.com/babel/babel/issues/2025.


This has already been fixed in Babel - maybe we should just update our build once they release instead?

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.

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.

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?

https://stackoverflow.com/questions/18875674/whats-the-difference-between-dependencies-devdependencies-and-peerdependencies

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?

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?

That sounds reasonable - let's just merge this and move on :)

ptomato

👍

ptomato · 2021-12-13T21:23:45Z

lib/intl.ts


 Object.defineProperty(DateTimeFormatImpl, 'name', {
-  writable: true,
+  writable: false,


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

Oh! I'll revert. I assumed this was a mistake. Serves me right for not checking the spec first.

ptomato · 2021-12-13T21:25:14Z

lib/index.ts

 import * as Intl from './intl';
 import { toTemporalInstant } from './legacydate';
+
+// Work around https://github.com/babel/babel/issues/2025.


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.

lib/intl.ts

12wrigja · 2021-12-14T20:29:52Z

lib/index.ts

+  Temporal.PlainDateTime,
+  Temporal.Duration,
+  Temporal.PlainMonthDay,
+  // Temporal.Now,


Either remove, or add a comment explaining why Now isn't also not-writeable?

I think it's because Now doesn't have its own prototype, it's a plain object, but agreed a comment would be good

Yep, that's why it's omitted. I'll add a comment and merge.

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

ptomato

👍

Port of tc39/proposal-temporal#1974

justingrant requested review from 12wrigja, Ms2ger and ptomato December 13, 2021 10:25

12wrigja approved these changes Dec 13, 2021

View reviewed changes

ptomato approved these changes Dec 13, 2021

View reviewed changes

justingrant force-pushed the no-writeable-prototypes-port-1974 branch from 381f234 to fd4c19c Compare December 14, 2021 05:56

Ms2ger removed their request for review December 14, 2021 08:05

12wrigja reviewed Dec 14, 2021

View reviewed changes

ptomato approved these changes Dec 14, 2021

View reviewed changes

Ensure Temporal prototypes aren't writable

6f3ca25

Port of tc39/proposal-temporal#1974

justingrant force-pushed the no-writeable-prototypes-port-1974 branch from fd4c19c to 6f3ca25 Compare December 14, 2021 22:47

justingrant merged commit 871d28d into js-temporal:main Dec 14, 2021

justingrant mentioned this pull request Dec 20, 2021

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

Open

Conversation

justingrant commented Dec 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptomato left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants