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

The textInfo getter seems unwell #62

Closed
bathos opened this issue Apr 27, 2022 · 5 comments
Closed

The textInfo getter seems unwell #62

bathos opened this issue Apr 27, 2022 · 5 comments

Comments

@bathos
Copy link

bathos commented Apr 27, 2022

A getter can behave this way:

let locale = new Intl.Locale("en-GB");

Object.is(locale.textInfo, locale.textInfo); // false

Then again, { ...obj } could send a fax. Neither the getter nor the trap would violate an invariant, but I’d still be surprised. :)

Getters like this in userland cause bugs when they meet something that expects values read with property access to constitute some meaningful aspect of state that would not have changed as a consequence of this typically “inert” act. For example the old Angular 1 digest loop would have choked on this API for that reason: no matter how many times it read locale.textInfo, it’d always be “dirty again” and an update would be triggered, etc.

Existing intrinsic getter behaviors are idempotent vis a vis the state associated with their receivers. It need not be a simple 1:1 relationship, but the values returned from [[Get]] do not change unless some related state changed. From a [[Get]] object method POV, the builtin getters implement the same relationship to the receiver’s state that the receiver’s own data properties already exhibit. If the textInfo getter produces novel state (especially new object identity) every time it’s evaluated, it’s may be returning results about the receiver’s state (method), it’s not modeling it (getter).

There’s no formalization or definition for this distinction to my knowledge. My attempt at describing the pattern may be pretty bad, but the pattern itself has been followed super consistently to date. There may not be a single exception to it in ECMA-262.

I’d suggest either of these alternatives:

  1. Change it to a method (align the API with the existing semantics)
  2. Change its behavior to returning a persistently associated frozen object (aligning the semantics with the existing API)
@FrankYFTang
Copy link
Collaborator

2. Change its behavior to returning a persistently associated frozen object (aligning the semantics with the existing API)

I prefer this. Would you care to propose a PR for this?

@FrankYFTang
Copy link
Collaborator

We discuss this issue in TG2 2022-10-06 and decide the right action is to bring this issue to TC39 in end of Nov to get feedback from TC39 and this issue block Stage 4 of this proposal .

@sffc
Copy link
Contributor

sffc commented Oct 6, 2022

@FrankYFTang
Copy link
Collaborator

We discuss this issue in the TC39 2022-11-30 meeting and many people express they like the change the from getter to methods better

FrankYFTang added a commit that referenced this issue Nov 30, 2022
Close #62

Per 2022-11-30 TC39 recommentations
webkit-early-warning-system pushed a commit to Constellation/WebKit that referenced this issue Dec 7, 2022
https://bugs.webkit.org/show_bug.cgi?id=248836
rdar://problem/103041601

Reviewed by Justin Michaud.

Nov 30 TC39 meeting decided to change Intl.Locale info getters to methods[1] because of object identity issue[2].
This patch aligns our implementation to this change.

[1]: tc39/proposal-intl-locale-info#65
[2]: tc39/proposal-intl-locale-info#62

* JSTests/stress/intl-locale-info.js:
(throw.new.Error):
(shouldBe):
(let.l.new.Intl.Locale.shouldBe):
* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/257449@main
@FrankYFTang
Copy link
Collaborator

TC39 2023-01-31 decide to go with PR67. Merged and close this ticket.

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 a pull request may close this issue.

3 participants