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

Should %Segments% instances have a link back to their Intl.Segmenter? #96

Closed
gibson042 opened this issue Jan 29, 2020 · 6 comments · Fixed by #117
Closed

Should %Segments% instances have a link back to their Intl.Segmenter? #96

gibson042 opened this issue Jan 29, 2020 · 6 comments · Fixed by #117

Comments

@gibson042
Copy link
Collaborator

It could be useful for e.g. iteration from an arbitrary point.

-function* segmentsAfterIndex( segments, index, segmenter ) {
+function* segmentsAfterIndex( segments, index ) {
   let match = segments.containing(index);
   if ( !match ) {
     return;
   }
   let offset = match.index + match.segment.length;
   let tail = segments.string.slice(offset);
-  for ( let tailSegment of segmenter.segment(tail) ) {
+  for ( let tailSegment of segments.segmenter.segment(tail) ) {
     tailSegment.index += offset;
     yield tailSegment;
   }
 }
@sffc
Copy link

sffc commented Jan 29, 2020

LGTM

@littledan
Copy link
Member

No particular concerns, but it also doesn't sound so bad to have to pass the segmenter around.

gibson042 added a commit to gibson042/proposal-intl-segmenter that referenced this issue May 29, 2020
gibson042 added a commit to gibson042/proposal-intl-segmenter that referenced this issue May 29, 2020
@gibson042
Copy link
Collaborator Author

gibson042 commented Jun 4, 2020

Options for avoiding the "exotic internal slot" hazard:

  1. Change segmenter from an accessor property on the prototype to a data property on the instance (Normative: Define Segments instance properties #108, matches proposal-promise-any errors property).
  2. Change segmenter from an accessor property on the prototype to a method that returns a fresh segmenter.
  3. Remove segmenter, leaving string.
  4. Remove both segmenter and string properties.

I don't like option 3 or 4 because they strip out too much from a Segments instance, and option 3 in particular is distasteful because having the string without the segmenter excessively limits what can be done with it. Option 2 can work but seems out of place. I personally favor option 1, in line with the decision from tc39/proposal-promise-any#38 .

@anba
Copy link

anba commented Jun 5, 2020

I prefer to wait for the broader discussion as suggested by @littledan before we try to reach consensus on any of the proposed options.

@gibson042
Copy link
Collaborator Author

Resolved in today's ECMA-402 call to sidestep the concern by removing both segmenter and string properties from segments instances, but preserving the ability to recover the input string by exposing it on iteration results per #116.

gibson042 added a commit to gibson042/proposal-intl-segmenter that referenced this issue Jul 10, 2020
gibson042 added a commit that referenced this issue Jul 10, 2020
…nt data objects

Improves correspondence with RegExp match objects
* Normative: Remove `string` accessor from %SegmentsPrototype%
* Normative: Add "input" to segment data objects

Fixes #116
Fixes #96
Closes #117
@littledan
Copy link
Member

In the July 16th SES call, @erights and I arrived at the conclusion that there is no need to avoid internal slots in cases like Intl.Segmenter and Temporal classes, where

  1. Internal slots are accessed just on the this value.
  2. No new "undeniables" (objects accessible from syntax) are being added
    Cases which run into one of these two constraints might be acceptable; it just takes more specific analysis. Intl.Segmenter does not run into these constraints, though.

If the change in #117 is justified by other aspects of good design, I'm fine with this decision . I don't think we should consider the "exotic internal slot hazard" a reason to make API design decisions, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants