Skip to content
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

Polyfill: Ensure that Temporal prototypes aren't writable #1974

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

justingrant
Copy link
Collaborator

This PR fixes #1965:

  1. For Intl.DateTimeFormat, sets writeable: false on prototype because it's created using function (which has a writeable prototype, unlike class which is not writeable).
  2. For Temporal classes, there's a Babel bug that causes prototypes to be writeable (Prototype object of ES6 classes always non-writable babel/babel#2025) and this PR works around it.

There are corresponding Test262 tests to check prototype writability here: tc39/test262#3344

@justingrant justingrant added the non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! label Dec 13, 2021
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #1974 (5c1d078) into main (4ef2748) will decrease coverage by 0.04%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1974      +/-   ##
==========================================
- Coverage   95.00%   94.96%   -0.05%     
==========================================
  Files          19       19              
  Lines       10941    10953      +12     
  Branches     1739     1738       -1     
==========================================
+ Hits        10394    10401       +7     
- Misses        531      535       +4     
- Partials       16       17       +1     
Flag Coverage Δ
test262 80.03% <44.44%> (-0.01%) ⬇️
tests 89.86% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
polyfill/lib/shim.mjs 68.75% <37.50%> (-31.25%) ⬇️
polyfill/lib/intl.mjs 100.00% <100.00%> (ø)
polyfill/lib/ecmascript.mjs 95.24% <0.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ef2748...5c1d078. Read the comment docs.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 13, 2021

I can release the Babel fix later today if needed.

justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Dec 13, 2021
This PR focuses on unblocking `strictNullChecks` and
`strictPropertyInitialization` in ecmascript.ts and calendar.ts.
It's intended as a complement to @jens-ox's work to remove
`GetIntrinsic`.

This PR:
* Enables strictNullChecks: true and strictPropertyInitialization: true
  in tsconfig.json.
* Improves calendar.ts TS types (this was most of the work in this PR)
* Removes all unnecessary use of `any` throughout the polyfill
* Updates types of ecmascript.ts and fixes a number of type bugs
* Ports several proposal-temporal PRs:
  * tc39/proposal-temporal#1975 - Refactors to
    align ecmascript.mjs with spec text
  * tc39/proposal-temporal#1974 - Ensure that
    Temporal prototypes aren't writable
  * tc39/proposal-temporal#1976 - Throw
    RangeError if there's an invalid offset string in
    ZonedDateTime-representing property bags
* Fixes a few index.d.ts types
* Refactors a few types in intl.ts
* A handful of trivial type changes in other files (ecmascript.ts and
  calendar-ts were 95%+ of the work).

I'd like to split out the runtime behavior changes (mostly the ported
PRs above) into a smaller PR which should make it easier to review
changes that can actually break things at runtime. I also need to port
tests over from tc39/proposal-temporal#1976.

Once those are split out, this PR should be ready to review.
@justingrant
Copy link
Collaborator Author

I can release the Babel fix later today if needed.

@nicolo-ribaudo - It's not urgent because I think we have a workaround, so the regular release schedule is fine for us. Thanks so much for jumping on this fix!

justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Dec 13, 2021
This PR focuses on unblocking `strictNullChecks` and
`strictPropertyInitialization` in ecmascript.ts and calendar.ts.
It's intended as a complement to @jens-ox's work to remove
`GetIntrinsic`.

This PR:
* Enables strictNullChecks: true and strictPropertyInitialization: true
  in tsconfig.json.
* Improves calendar.ts TS types (this was most of the work in this PR)
* Removes all unnecessary use of `any` throughout the polyfill
* Updates types of ecmascript.ts and fixes a number of type bugs
* Ports several proposal-temporal PRs:
  * tc39/proposal-temporal#1975 - Refactors to
    align ecmascript.mjs with spec text
  * tc39/proposal-temporal#1974 - Ensure that
    Temporal prototypes aren't writable
  * tc39/proposal-temporal#1976 - Throw
    RangeError if there's an invalid offset string in
    ZonedDateTime-representing property bags
* Fixes a few index.d.ts types
* Refactors a few types in intl.ts
* A handful of trivial type changes in other files (ecmascript.ts and
  calendar-ts were 95%+ of the work).

I'd like to split out the runtime behavior changes (mostly the ported
PRs above) into a smaller PR which should make it easier to review
changes that can actually break things at runtime. I also need to port
tests over from tc39/proposal-temporal#1976.

Once those are split out, this PR should be ready to review.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Dec 13, 2021
This PR focuses on unblocking `strictNullChecks` and
`strictPropertyInitialization` in ecmascript.ts and calendar.ts.
It's intended as a complement to @jens-ox's work to remove
`GetIntrinsic`.

This PR:
* Enables strictNullChecks: true and strictPropertyInitialization: true
  in tsconfig.json.
* Improves calendar.ts TS types (this was most of the work in this PR)
* Removes all unnecessary use of `any` throughout the polyfill
* Updates types of ecmascript.ts and fixes a number of type bugs
* Ports several proposal-temporal PRs:
  * tc39/proposal-temporal#1975 - Refactors to
    align ecmascript.mjs with spec text
  * tc39/proposal-temporal#1974 - Ensure that
    Temporal prototypes aren't writable
  * tc39/proposal-temporal#1976 - Throw
    RangeError if there's an invalid offset string in
    ZonedDateTime-representing property bags
  * tc39/proposal-temporal#1977 - Refactor
    and fix non-ISO calendars in polyfill
* Fixes a few index.d.ts types
* Refactors a few types in intl.ts
* A handful of trivial type changes in other files (ecmascript.ts and
  calendar-ts were 95%+ of the work).

I'd like to split out the runtime behavior changes (mostly the ported
PRs above) into a smaller PR which should make it easier to review
changes that can actually break things at runtime. I also need to port
tests over from tc39/proposal-temporal#1976.

Once those are split out, this PR should be ready to review.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Dec 14, 2021
This PR fixes tc39#1965:
1. For Intl.DateTimeFormat, sets `writeable: false` on  `prototype`
   because it's created using `function` (which has a writeable
   `prototype`, unlike `class` which is not writeable).
2. For Temporal classes, there's a Babel bug that causes prototypes to
   be writeable (babel/babel#2025) and this PR
   works around it.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Dec 14, 2021
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

justingrant added a commit to js-temporal/temporal-polyfill that referenced this pull request Dec 14, 2021
@nicolo-ribaudo
Copy link
Member

Fyi, we released the Babel fix yesterday.

@justingrant justingrant merged commit ca4a9e0 into tc39:main Dec 14, 2021
@ptomato ptomato added the no-spec-text PR can be ignored by implementors label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prototype property of Temporal types should not be writable
3 participants