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

Editorial: Simplify object snapshotting #2586

Merged
merged 3 commits into from
May 29, 2023

Conversation

gibson042
Copy link
Collaborator

@gibson042 gibson042 commented May 25, 2023

Introduces SnapshotOwnProperties to precede CopyDataProperties with creating the target object and follow it with returning that object, collapsing many common algorithm subsequences (including all of those using the CopyOptions operation, which is fully subsumed).

Once introduced to ECMA-262, it can also simplify RestDestructuringAssignmentEvaluation and RestBindingInitialization:

-          1. Let _restObj_ be OrdinaryObjectCreate(%Object.prototype%).
-          1. Perform ? CopyDataProperties(_restObj_, _value_, _excludedNames_).
+          1. Let _restObj_ be ? SnapshotOwnProperties(_value_, %Object.prototype%, _excludedNames_).

@gibson042 gibson042 requested a review from ptomato May 25, 2023 18:34
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #2586 (8f31100) into main (dac765f) will increase coverage by 96.14%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main    #2586       +/-   ##
=========================================
+ Coverage      0   96.14%   +96.14%     
=========================================
  Files         0       20       +20     
  Lines         0    11364    +11364     
  Branches      0     2164     +2164     
=========================================
+ Hits          0    10926    +10926     
- Misses        0      375      +375     
- Partials      0       63       +63     
Impacted Files Coverage Δ
polyfill/lib/calendar.mjs 87.77% <100.00%> (ø)
polyfill/lib/ecmascript.mjs 98.28% <100.00%> (ø)
polyfill/lib/zoneddatetime.mjs 100.00% <100.00%> (ø)

... and 17 files with indirect coverage changes

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.

Looks like a good improvement!

Would you like to propose the change to rest parameter destructuring in 262 separately? That would help make the Temporal proposal a bit smaller and get some editor feedback on this operation.

@gibson042
Copy link
Collaborator Author

Good idea: tc39/ecma262#3081

@@ -1505,12 +1505,8 @@ <h1>Temporal.Calendar.prototype.mergeFields ( _fields_, _additionalFields_ )</h1
<emu-alg>
1. Let _calendar_ be the *this* value.
1. Perform ? RequireInternalSlot(_calendar_, [[InitializedTemporalCalendar]]).
1. Set _fields_ to ? ToObject(_fields_).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it ToObject and not "If fields is not an Object, throw a TypeError" ?

And was it an accidentally normative change to remove it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To answer the second question first, this change is not normative because the first observable consequence of SnapshotOwnProperties is ? ToObject(_source_) in step 3.

As for the first question, I don't know why mergeFields includes coercion to object but I would be in favor of replacing that with rejection of non-objects in concordance with #2574 (and such a change could be implemented in SnapshotOwnProperties itself).

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 18, 2023
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jul 19, 2023
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jul 21, 2023
…ewers,dminor

Per <tc39/proposal-temporal#2586>.

Differential Revision: https://phabricator.services.mozilla.com/D182055

UltraBlame original commit: 1a2a324c251824b802d2f635171b5223022dc671
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jul 21, 2023
…ewers,dminor

Per <tc39/proposal-temporal#2586>.

Differential Revision: https://phabricator.services.mozilla.com/D182055

UltraBlame original commit: 1a2a324c251824b802d2f635171b5223022dc671
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jul 21, 2023
…ewers,dminor

Per <tc39/proposal-temporal#2586>.

Differential Revision: https://phabricator.services.mozilla.com/D182055

UltraBlame original commit: 1a2a324c251824b802d2f635171b5223022dc671
ptomato added a commit that referenced this pull request Sep 8, 2023
These are mistakes from #2586. Spotted by Anba.

Closes: #2620
ptomato added a commit that referenced this pull request Sep 8, 2023
These are mistakes from #2586. Spotted by Anba.

Closes: #2620
@ptomato ptomato mentioned this pull request Sep 8, 2023
Ms2ger pushed a commit that referenced this pull request Sep 12, 2023
These are mistakes from #2586. Spotted by Anba.

Closes: #2620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants