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

Simpler solution to what Number.range aims to solve #63

Closed
ogbotemi-2000 opened this issue Feb 18, 2023 · 50 comments
Closed

Simpler solution to what Number.range aims to solve #63

ogbotemi-2000 opened this issue Feb 18, 2023 · 50 comments

Comments

@ogbotemi-2000
Copy link

A non-opinionated approach to what the Number.range proposal aims to solve in terms of

  • amount of code,
  • prototype pollution,
  • speed of execution,
  • avoidance of the overhead of using generators,
  • compatibility and extensibility

You can give the variables more precise and intuitive names

//make x have a unique value
const x = Symbol ? Symbol() : 'd41e9d5d-164b-4f1c-8e20-ea2d54341fc0'; 

Object.defineProperty(Number.prototype, x, {
  get() {
    let arg=[], n=this.valueOf(), i=0;
    for(; i++<n; arg[i-1]=i-1); return arg;
  }
});
200[x] // [0, 1, ..., 199]
5[x] // [0, 1, ..., 4]

Variations

The code above generates arrays in ascending order. Hence,

  • For descending order
const _x = Symbol ? Symbol() : '01be628c-d93d-421d-8a18-41208b3603fd'; 

Object.defineProperty(Number.prototype, _x, {
  get() {
    let arg=[], i, n = this.valueOf();
    for(i=n; arg[i-n]=n--;); return arg;
  }
})
5[_x] // [4, ..., 1, 0 ]

For stepped outputs

  • In descending order
const _x_x = Symbol ? Symbol() : 'ea172ec0-b81b-4428-a250-de6a99ba7783'; 

Object.defineProperty(Number.prototype, _x_x, {
  get() {
    let arg=[], i=0, arr = (this.valueOf()+'').split('.'), n=arr[0], step=arr[1]||1
    for(;  (n-=step)>0; arg[i++]=n); return arg;
  }
})
5[_x_x] // [4, ..., 1, 0]
5.2[_x_x] // [3, 1]
5.3[_x_x] // [2]
/* Generating odd numbers between 0 and 9 is as simple as */
9.2[_x_x] // [7, 5, 3, 1]
  • In ascending order
const x_x = Symbol ? Symbol() : 'f66aed03-fed5-4616-800b-2571ca2ce152'; 

Object.defineProperty(Number.prototype, x_x, {
  get() {
    let arg=[], i=0, arr = (this.valueOf()+'').split('.'), n=arr[0], step=arr[1]||1, len = n/step|0;
    for(;  (n-=step)>0; arg[len-++i]=n); return arg;
  }
})
5.2[x_x] // [1, 3]

Perf tests

Using console.time yielded very interesting results

console.time(); 900[x_x]; console.timeEnd()
// between 0.1ms to 0.04 ms when ran multiple times. Likewise for the other variations
@ljharb
Copy link
Member

ljharb commented Feb 19, 2023

Are you suggesting that users should modify objects they don’t own? Symbols don’t magically make that an OK thing to do, it just reduces the likelihood of collisions.

If it’s a builtin symbol, it’s not super ergonomic or discoverable or readable imo, compared to Number.range

@Jack-Works
Copy link
Member

Hello, your API design does not follow the JavaScript built-in API convention. We previously don't have this style of API.

About the performance, I believe engines can handle it.

@ogbotemi-2000
Copy link
Author

Hello @ljharb, I will answer each paragraph, as quoted above, below

"Are you suggesting that users should modify objects they don’t own? Symbols don’t magically make that an OK thing to do, it just reduces the likelihood of collisions."

Modification of objects not owned ?

No, nada, I do not intend to suggest any modification of objects and you may agree with me that syntaxes such as Number[x] do not hint any modification of the Number object because x is unique as seen in my comment above.
If otherwise, then the very crux of this discussion - Number.range definition, may be seen as suggesting modification of objects not owned.

I wanted to point out aspects of the Number.range current definition which can lead to its code being split to lesser ones because it tries to consider a lot of scenarios (kudos for the that by the way) in one function body

"If it’s a builtin symbol, it’s not super ergonomic or discoverable or readable imo, compared to Number.range"

Unreadability of defined property on Number.prototype

If you can be so kind as to go through the points I listed at the start of my comment above, you will see the term prototype pollution which is a security risk because <Object>.prototype is available to alterations.

Using a Symbol or a UID as the key of an object property makes such alterations hard if not impossible. Also the uniqueness of the said object property's key prevents catastrophes like SmooshGate from easily occurring on a whim without using Object.freeze

@ogbotemi-2000
Copy link
Author

Hello @Jack-Works, I do not intend to go against the JavaScript API convention.

I only wrote code that is easy to understand, precise, fast and extensible in the case of abstractions that conform to API standards as illustrated below

const x = Symbol ? Symbol() : 'd41e9d5d-164b-4f1c-8e20-ea2d54341fc0';
Object.defineProperty(Number.prototype, x, {
  get() {
    let arg=[], n=this.valueOf(), i=0;
    for(; i++<n; arg[i-1]=i-1); return arg;
  }
});

5[x].forEach(function(e, i){
  /**/
}),
5[x].map(function(e, i){
  /**/
})

That said, I look forward to suggestions related to improvements and conformity of code from you.
Peace

@ljharb
Copy link
Member

ljharb commented Feb 20, 2023

@ogbotemi-2000 x being unique doesn't change that you're installing it on Number yourself. The language owns its own builtins, and it can modify them all it likes; you do not own them, and thus should not modify them in any way.

Prototype pollution imo is simply not worth preventing if the cost is suffering the ergonomics loss of non-string method access for common operations - and in the case of Number.range, there's no risk because it's not a prototype method (and Number isn't a prototype object anyways).

@ogbotemi-2000
Copy link
Author

ogbotemi-2000 commented Feb 25, 2023

Thanks @ljharb, I finally understood what you meant by not modifying objects I do not own.
However, your statement - " ...suffering the ergonomics loss of non-string method access for common operations", by my judgement, advices that tried-and-true code syntaxes used for strings should not be borrowed into that for numbers.

This is ideal but not totally possible at best because the primitive types number and string are actually objects that have convenient methods defined on their respective constructor's prototype for the said methods to be available to every instance of Number or String.

I believe the prototype object is available on some objects is JavaScript: Number, Array, String among others to offer a stable, safe way of adding features not present to the language.

You're right about the uncertainty as regards builtins in JavaScript but, I must say, if JavaScript has evolved and matured to the point where features such as

Array.prototype.map
Array.prototype.forEach

are available thanks to ECMAScript then, I think adding features that respect the conditions below should be done albeit sparingly.

  • does not override existing properties in any way
  • is readable and easily extensible
  • has no side effects; point-free
  • does not make the object loose its perks in any way
  • fills a need

You may add more to the list above if you resonate with me

@ljharb
Copy link
Member

ljharb commented Feb 25, 2023

Those conditions are fully satisfied by static methods on the constructors.

@tabatkins
Copy link

Additionally, if the official proposal were to adopt these, the symbols would need to have reasonable names and be placed in some namespace anyway. So you wouldn't be writing 5[_x], you'd be writing 5[Number.descendingRange] or something, which is at best equivalent to the current syntax using static methods. (But in reality it's decently worse, I think.)

@tabatkins
Copy link

" ...suffering the ergonomics loss of non-string method access for common operations", by my judgement, advices that tried-and-true code syntaxes used for strings should not be borrowed into that for numbers.

Jordan wasn't referring to methods on strings, he was referring to methods whose names are strings, rather than Symbols. String-named methods are much more ergonomic to use, because you get to use the foo.bar() syntax rather than foo[bar], plus you don't need to store the name on some namespace object, like you have to do with Symbols.

@ogbotemi-2000
Copy link
Author

What you say is true @tabatkins.
Given how unwieldy my approach is, I had hoped the promising results of console.time'ing my code would convince @Jack-Works to avoid the overhead of using generators where possible and stick to the good ol' regular for loop.

Maybe the standards-compliant aspects of my code can be used given its brevity and conciseness in the say, quick generation of arrays that will not be modified immediately, i.e. a sequence.

I believe it stands to reason that such code as mine above might be used internally albeit sparingly for performance gains and not for code portability which Number.range achieves.

@Jack-Works
Copy link
Member

Well, if performance concerns are really that important (note: iterating array with for of is also using generators theoretically), maybe we may add back Number.range() (this proposal is currently renamed as Iterator.range()) in the future

@ogbotemi-2000
Copy link
Author

Iterator.range is a better choice of name if it returns an Iterator that can be iterated over via for of or Array.from.

As you mentioned earlier, "... I believe the engines can handle it" too if, in my opinion, the points and code below is considered

Performant iterations may be achieved if the internally used object {value: <value ± step>, done: false} is used only to yield a value when needed and not to make an array out of the values

Maybe providing a callback that gets called with yielded each value may improve performance whereby the operation by the callback on each value happens immediately and not delayed in the case of [/*generated array*/].forEach(callback)

A likely implementation Iterator.range({start, end, step, callback})


If that isn't possible then you may circumvent the overhead of using generators by using an ArrayIterator which may be used as follows

const arrayIterator = Object.create([][Symbol&&Symbol.iterator||'values']().__proto__, {next:
      {value:_=>{ 
      /**/ }
  });

//which is then refactored into a function below
Iterator.range = function(obj) {
     // you can implement your trusted code to make sanity checks on obj here

    this.__itr||=Object.create([][Symbol&&Symbol.iterator||'values']().__proto__, {next:
      {value:_=>{
        /*define your implementation here with more control over the internally used {value: Number, done: Boolean} object */
        return conditionMet ? {value: undefined: done:true} : {value: value, done:false} 
    }}});
    return this.__itr; //stored as an object property in order for it to outlive this context
  } 

the this.__itr||=... operation above is very important for performance because

It prevents an ArrayIterator from being created more than once when Iterator.range is called without the
new keyword.

This has the effect of making previous variables that store a reference of the by now shared ArrayIterator reflect the changes made to the said ArrayIterator returned by the latest invocation of Iterator.range.

let a = Iterator.range({start: 0, stop: 5, step: 1}); a,next() yields 1.
let b =Iterator.range({start: 2, stop: 10, step: 2}); b.next() yields 4, a.next() yields 6;

This behavior which is due to a closure may be useful in the case of updating state with new data and making that data reflect in previous states without calling a function to do so.

Calling Iterator.range with the new keyword prevents this behavior and it is similar to simply removing the || operator albeit faster because an ArrayIterator will not be created every time Iterator.range gets called

@ogbotemi-2000
Copy link
Author

I am closing the issue given the lull in code reviews in this thread.

@ogbotemi-2000 ogbotemi-2000 reopened this Jul 23, 2023
@ogbotemi-2000
Copy link
Author

Reopened to make the closing of this issue unanimous

@ogbotemi-2000
Copy link
Author

I look forward to comments regarding the use of ArrayIterator in lieu of a generator in the function body of Iterator.range

@jpolo
Copy link

jpolo commented Aug 19, 2023

IMHO, Iterator.range is not the right name, as there is a BigInt and potentially a Decimal and/or BigDecimal in the future. The concept of range is tight with the type of value not Iterator per se.

I can understand that the number range is special because could be used to iterate over array indices but it should not a reason to attach it on the Iterator class...

@Jack-Works
Copy link
Member

IMHO, Iterator.range is not the right name, as there is a BigInt and potentially a Decimal and/or BigDecimal in the future. The concept of range is tight with the type of value not Iterator per se.

I can understand that the number range is special because could be used to iterate over array indices but it should not a reason to attach it on the Iterator class...

I intended to make Iterator.range a polymorphic function. This is done in #59, which means we can support Decimal/BigDecimal/Date/Temporal/... in the future. Iterator.range(1m, 10m) // decimal

This decision is further based on the discussion in #17, both sides Iterator/Iterable have objections and I cannot be convinced by either side. To make this proposal happen instead of dying in the discussion, I choose to explicitly mention Iterator in the API name to solve the concern with Iterator mentioned in #17.

@jpolo
Copy link

jpolo commented Aug 22, 2023

Ok I switch to the #17 to continue this discussion.

@Jack-Works
Copy link
Member

#17 is another problem, but that is why I changed this to a polymorphic function. I don't think polymorphic function is that bad. It also can be polyfilled.

@ogbotemi-2000
Copy link
Author

ogbotemi-2000 commented Sep 1, 2023

@Jack-Works Hello, it's been quite a while.

About the polymorphic behavior of Iterator.range, I'd like to know whether you are handing off the actual generation of values to some internal operation or implementing it yourself in a classic yield <increments> style.

Iterator.range does not appear to be a method available to the instances of Number for which it was originally conceived as Number.range and, I'm afraid I cannot see how you will truly implement polymorphism for types such as BigDecimal, Date, etcetera, if Iterator.range still remains as a static method of the Iterator class or object?

It may be that Iterator.range's polymorphism will only be enforced via the argument-checking heuristics you pull off in knowing the type of the argument passed to it.

Besides, the trouble of creating an Iterator class (drives polymorphism home in good'ol Java style than object) with either an instance or static method range just to iterate over an array, collection via special and internally used

{
 next: {
  value: any
  done: Boolean
  }
}

object or otherwise is just too much for what can already be achieved through the

for(entry of entries) {

}

I would like to suggest that you consider ArrayIterator instead since arrays can store any type and it is implemented by collections - StringIterator, MapIterator that are all already available in the browser.

  • You get native iterative behaviour out of the box and can create NumberIterator to treat +new Date as a number in milliseconds from January 1st UTC, 1970 and return the increments as new Date(<NumberIterator.next>) thereby supporting
    both BigInt and Date.
  • You could even use ArrayIterator to avoid code bloat and aid some internal Date to BigInt conversion, should the need arise, since they are all handled in the same place via arrays which safely handles precision and multiple.

Decimal and BigDecimal may involve further considerations about whether the trouble of place values should be considered and also requiring defining what you want increment in a "range" to be - 1 or 0.1 or 0.01

My last comment before I initially closed this thread extensively addresses how you can implement iteration functionality via the elusive ArrayIterator.

Good day and well done

@Jack-Works
Copy link
Member

It may be that Iterator.range's polymorphism will only be enforced via the argument-checking heuristics you pull off in knowing the type of the argument passed to it.

Yes, this is what I intended to do.

Besides, the trouble of creating an Iterator class (drives polymorphism home in good'ol Java style than object) with either an instance or static method range just to iterate over an array, collection via special and internally used

I'm sorry I can't understand this section clearly. The Iterator global is not brought by this proposal, but by https://github.com/tc39/proposal-iterator-helpers, if your question is about this.

I would like to suggest that you consider ArrayIterator instead since arrays can store any type and it is implemented by collections - StringIterator, MapIterator that are all already available in the browser.

You get native iterative behaviour out of the box and can create NumberIterator to treat +new Date as a number in milliseconds from January 1st UTC, 1970 and return the increments as new Date(<NumberIterator.next>) thereby supporting
both BigInt and Date.
You could even use ArrayIterator to avoid code bloat and aid some internal Date to BigInt conversion, should the need arise, since they are all handled in the same place via arrays which safely handles precision and multiple.

Also this part. If you can explain it more clearly it would be good!

@ogbotemi-2000
Copy link
Author

  • Okay, your first answer - "Yes, this is what I intended to do." will work just fine for all except floating point numbers, "Big" or small

  • My bad, I guess "Iterator" is used in this thread as a placeholder for an iterable. What had erroneously horrified me before was that I thought you wanted to create an Iterator class that gets extended by Date, BigDecimal (via the prototype chain, maybe?) and makes range available for implementation by its inheritors.

My background in Java and your quite wrong and titular use of polymorphism as an adjective made me go for a toss 😀.

  • Now the meat of the discussion, I need you to read my very last comment before I had initially closing this issue as I mentioned at the end of my last message today - "My last comment before I initially closed this thread extensively addresses how you can implement iteration functionality via the elusive ArrayIterator. ..."

It will provide you the context in which you will ask the right questions.

@Jack-Works
Copy link
Member

Let me assume by ArrayIterator you mean this:

img

I don't see how I can "implement via" ArrayIterator in a different way than the current proposal because it is basically the same as Iterator global object.

@ogbotemi-2000
Copy link
Author

The image present in your message failed to load @Jack-Works

Apologies for the 3-day delay, I got sidetracked.

Try this, test both codes - my ArrayIterator alternative and your Iterator proposal through multiple inputs and see the performance results.

I'd like to "see" the code for Iterator, care to drop a link?

You will realize that codes may look the same but are totally different and it is the aim of this by now long-ass issue to offer a 'simpler [and faster] solution...' to what this repository aim to solve.

@tabatkins
Copy link

I also have no idea what your proposal actually is. Your code that attempted to demonstrate this "ArrayIterator" was both too incomplete to understand, and contained a bunch of inline error-handling (like ternaries to handle the possibility of an old JS that didn't know Symbols) that obfuscated the meaningful code. It also looks like it's modifying global state so you can only have one iterator in your code at a time?

@Jack-Works
Copy link
Member

I also have no idea what your proposal actually is. Your code that attempted to demonstrate this "ArrayIterator" was both too incomplete to understand, and contained a bunch of inline error-handling (like ternaries to handle the possibility of an old JS that didn't know Symbols) that obfuscated the meaningful code. It also looks like it's modifying global state so you can only have one iterator in your code at a time?

+1, I suggest you rewrite your code, to split polyfill and user code. How do you expect to polyfill your design? How will users use it?

@ogbotemi-2000
Copy link
Author

I see that both @Jack-Works and @tabatkins have incorrectly understood my code and this is why I am sending a working and concise version below:

Iterator.range = function(num) {
    console.log(this)/*logs Iterator and not the global object or window */

  /* the if condition below assigns this.breakClosure to a value of num coerced into a number using the
   * "+" operator; the result of which is checked to be a truthy. If otherwise, the program exits
  */
  if(!(this.breakClosure = +num)) return;
   /* the assignment operation below occurs only once - when this._itr is not yet defined
    * this is done so as to prevent Object.create from creating a new ArrayIterator everytime when
    * arrayIterator is called
    */
    this._itr||=Object.create([].values(), {next:
      {value:_=>{
        /* this.breakClosure below is needed here so as to update its value below by reference to the
         * num argument above: if it were a variable such as num and not an object property,
         * the closure in this arrow function context traps its value to the very first value of the num
         * argument above
         */
      /* the decrement operation on this.breakClosure makes this._itr  yield
        * values in a descending order when iterated over
        */
        return --this.breakClosure>-1
          ? {value:this.breakClosure, done:false}
          : {value:(this.breakClosure=void 0), done:true}


          /* you are in charge of what should turn up when the iterator is exhausted
           * i.e. by setting this.breakClosure = 0 or null or, in my case, undefined
           * interestingly not wrapping this.breakClosure=void 0 in brackets above makes the
           * JavaScript VM evaluate it as null
           * (coercing an object property set to undefined to be null, I guess)
          */
    }}});
    return this._itr
  }

You may add support for start, end, step arguments to it.
It returns an iterator that yields values in a descending order from the value of num to 0.

As regards polyfilling, [].values() is supported by modern browsers,
Here are the performance gains:

  • by using this syntax this._itr||=, the ArrayIterator gets created once whenever Iterator.range(num) is called without the new keyword.
  • using the native ArrayIterator as an iterator guarantees native iterative behaviour which is desirable to having to implement it through yield.

Examples

let itr    = Iterator.range(5),
     itr_1 = new Iterator.range(10),
     itr_2 = Iterator.range(7);

itr.next() // {value: 4, done: false}
Array.from(itr) // [3, 2, 1, 0], An iterable iterator.
itr.next() // {value: undefined, done: true} 

itr_1.next() // {value: 9, done: false}, due to new keyword which creates a new "this" context
itr_2.next() /* {value: undefined, done: true}, automatically reflects the state of this._itr which had been modified by itr,
may be useful in reflecting changes in some state in either vanilla JavaScript or React without having to listen to events or
call a function
*/

Why I sent incomplete code before initially closing this issue is because I want to leave its implementation details up to you devs and only discuss the core of the code.

@Jack-Works
Copy link
Member

Hi, I read your example. I don't know why you design it in a descending way by default, it looks not easy to use when you want it to be ascending.
I also do not see any big difference with the current proposal, can you point out which part of the current proposal you don't like?

@ogbotemi-2000
Copy link
Author

Man, I put it there so that you may implement it however you wish. Descending or ascending isn't the point

Differences

...
Object.defineProperty(Iterator, "range", {
        configurable: true,
        writable: true,
        value: (start, end, option) => {
            if (typeof start === "number") return new NumericRangeIterator(start, end, option, SpecValue.NumberRange)
            if (typeof start === "bigint") return new NumericRangeIterator(start, end, option, SpecValue.BigIntRange)
            throw new TypeError("Iterator.range only supports number and bigint.")
        },
    })
...

Needlessly verbose and large code to implement Iterator.range even with all safety and sanity checks removed.
I mean, you can easily obtain an internal Iterable that has a next method defined by using [].values() or [].keys() as follows:

Object.create([].values(), {
{
  next:{
    value:_=>{/*return {value:<Number>, done:<Boolean>} based on internal conditions */ }
  }}
})

Which gives more control and code visibility than the esoteric Object.getPrototypeOf(Object.getPrototypeOf((function* () {})())), since they both function the same way, just to obtain an iterable's next method that still gets called the same way internally by for(let i of j) {/**/} loops and Array.from(Iterable).

I guess I'll leave this issue open if you fail to see the bloat in the source code of your implementation of Iterator.range.
Making a separate fork is not needed since apparently everyone else is on the other side of the seesaw.

And doing so won't make the TC39 committee see the bloat since even they apparently approved it in the first place.

It has been a good run, gentlemen.
Good day.

@ljharb
Copy link
Member

ljharb commented Sep 6, 2023

It seems like you're focusing on the implementation of the method, which isn't all that important (as long as it's possible to implement the spec reasonably). What matters is the user-visible design and API.

@ogbotemi-2000
Copy link
Author

ogbotemi-2000 commented Sep 7, 2023

I hear you, making the source code lesser and easier will make it easier and transparent when adding improvements down the line (you do have that in mind, right?) while still maintaining the "user-visible" design and API you speak of.

I won't wanna use some slow addition to ECMAScript like Iterator.range with such source code then, native or not

@tabatkins
Copy link

No, making the source code slightly smaller will not meaningfully impact our ability to change things down the line.

It also still appears that your code is modifying global state by default if you call it without new. What is the use-case for this?

@ogbotemi-2000
Copy link
Author

No, no global state modified in any way - this represents Iterator and not window Did you run it? There is a reason why I included console.log(this) in the most recent implementation I sent Jack-Works.

@ogbotemi-2000
Copy link
Author

You call that change slightly small? @tabatkins, Nope, it ain't at all and if it is, do explain

@tabatkins
Copy link

...yes, Iterator is global state.

@ogbotemi-2000
Copy link
Author

In fact, the core of the bloat in Iterator.range revolves around

const origNext = Object.getPrototypeOf(Object.getPrototypeOf((function* () {}).next

which then gets closed over in the functions using it.
How is it implemented?

origNext.call()

_we will just ignore why the proposal chose an approach that further costs performance via .call() for now, right? _
where you could have easily have both creation and implementation in one place as follows:

Object.create([].values(), {next:
      {value:_=>{ /*return {value: <whatever>, done:<Boolean>}*/ }
      }
})

which offers lesser code, determinism and fine-grained control over how and what happens.

At this point, it is as though there is a smear that covers the eyes of the delegates - @ljharb, @Jack-Works, and prevents them from seeing what is plainly a benefit.
Maybe it is the hassle and [excess] bureaucracy involved in changing the current proposal and presenting to TC39

@ogbotemi-2000
Copy link
Author

😂😀 That is hilarious @tabatkins, I know you mean well and perhaps you may enlighten me on how Iterator in Iterator.range is a global state. (I mean, global had to have a different meaning to you )

@ljharb
Copy link
Member

ljharb commented Sep 7, 2023

Iterator is a globally available object, so any mutations to it are global state.

Additionally, your tone is becoming hostile. Please familiarize yourself with our Code of Conduct.

@ogbotemi-2000
Copy link
Author

Okay, I had it wrong then.
I guess there is no harm done in mutating Iterator by adding properties to it - essentially treating it as the object that it is.

@ljharb
Copy link
Member

ljharb commented Sep 7, 2023

There is always huge harm in mutating objects you don’t own.

@Jack-Works
Copy link
Member

Jack-Works commented Sep 7, 2023

Which gives more control and code visibility than the esoteric ...

If you take a look at the spec text of Map.prototype.get(https://tc39.es/ecma262/#sec-map.prototype.get), you will find it is using an O(N) algorithm, which will never be used on a production-ready implementation of JavaScript engine.

If the API design does not prevent V8, SpiderMonkey, or other engines from implementing it fast, it's definitely OK to use a "slower" spec text. The spec text just writes down the expectations of the API design.

const origNext = Object.getPrototypeOf(Object.getPrototypeOf((function* () {}).next

About this part, yes I changed the prototype of Generator.prototype.next, it also slows down the execution of generators, but this polyfill is NOT for production use. I changed Generator.prototype.next because I want to make a polyfill that has observable difference with a real native implementation of Iterator.range in any sense (if you are curious why this is necessary for a 100% real polyfill, please leave a comment, I'm happy to explain why). This is only for anyone who wants to debug it. A real JS engine can do this in a more efficient way, also a polyfill.

global state

By global state, we mean information can be shared between two different programs that don't have direct contact (for example, hold objects/functions created by the other side) with each other.

Yes, I don't see you're exposing/modifying the global state, if I understand you correctly, you're trying to write a very loose polyfill (for example, you leaked breakClosure internal variable) for the same API. This problem is not what we usually care especially since this is a step-by-step polyfill.

Also, if you care about polyfill performance this much, maybe you should also not use core-js (https://github.com/zloirock/core-js/blob/bc87aaee825d8131d64608d51f9a5faf1eaa0004/packages/core-js/internals/numeric-range-iterator.js)

@tabatkins
Copy link

tabatkins commented Sep 7, 2023

The global state modification was how they directly modified this without verifying that it was actually a new object; if called without new it would thus cache state on Iterator itself. You can see this in the snippet they provided (I'll trim to the relevant bits):

let itr    = Iterator.range(5),
     itr_2 = Iterator.range(7);

itr.next() // {value: 4, done: false}
Array.from(itr) // [3, 2, 1, 0], An iterable iterator.
itr.next() // {value: undefined, done: true} 

itr_2.next() /* {value: undefined, done: true}, automatically reflects the state of this._itr which had been modified by itr, */

@Jack-Works
Copy link
Member

close for now since this is not a real problem, discussions are still welcome

@Jack-Works Jack-Works closed this as not planned Won't fix, can't repro, duplicate, stale Sep 8, 2023
@ogbotemi-2000
Copy link
Author

Hello @Jack-Works please explain why the extra measure of offering an observable difference between the polyfill this repo is for - Iterator.range and some other implementation of it is necessary as mentioned in your comment:
#63 (comment)

Also, I'd like to know why you went through the trouble of storing next in order to present it as a polyfill when it is not because it only works in browsers that already support generators and not older browsers as a 'polyfill' should.

@ljharb "There is always huge harm in mutating objects you don’t own."
Woah, now I am even lost, this line of code in the spec Object.defineProperty(Iterator, 'range'... meant that Iterator is owned by the programmer and not Object because he created it on Object otherwise, defining range on Iterator as the code presents is also harmful in that regard since he "does not own it"

@ogbotemi-2000
Copy link
Author

@Jack-Works, you could have used the Object.create(<ArrayIterator>...) that I created if you wanted to present a polyfill first because it is supported in older browsers and offers lesser code scan for other devs "who wants to debug it" as you said.

Global state
I really do not enjoy going against what more that one person has said but, I must iterate (pun absolutely intended 😉).
Firstly, from your definition of global @Jack-Works - _"...information can be shared between two different programs that don't have direct contact (for example, hold objects/functions created by the other side) with each
other", the only 'side' is the object Iterator which isn't global (say window, global, etc) and which is this in Iterator.range and, on which I further defined _itr meant for internal use (as denoted by the underscore), and breakClosure so as to break the closure created as shown below

{
  next: {
  value: _=>/* closure created here */
  }
}
...

I am aware that Jack-Works partly thinks so too given his applaud-worthy, non-partisan observation: "Yes, I don't see you're exposing/modifying the global state, if I understand you correctly...".
I do not modify global objects, aye, 'tis in me pirate codex, in my code and if objects are now global states to their functions and properties then that is quite a shocker.

breakClosure is

  • not leaked in any way - it 'lives', 'dies' and mutates in Iterator.range. It is only cached as this.breakClosure`
  • actually what gets decremented in each {value, done}
  • only defined on this - Iterator in order to "outlive this(more puns, please ^_^) context" for when it gets decremented
  • modifying the numbers generated by my Iterator.range(5) does not modify Iterator.breakClosure or any state in Iterator because it is a primitive
  • I apologize for the abstruse approach I used, it was a result of minutes of constant debugging and testing for efficiency and brevity

I know that seeing this all over the place automatically creates a bias but, fellow developers, please go through it. There is more to it and I am will explain to and also learn from you too.

@Jack-Works
Copy link
Member

@ogbotemi-2000

because it only works in browsers that already support generators

because it is supported in older browsers

This polyfill does not care for compatibility with old browsers. It just implements the proposal in the mainline browsers.

There is always huge harm in mutating objects you don’t own.

Yes. Polyfill is an exception when it implements things correctly.

lesser code scan for other devs "who wants to debug it"

It's very clear that the core of the polyfill is NumericRangeIterator internal class and NumericRangeIteratorObject internal function.

the object Iterator which isn't global

It is (not now). This proposal is intended to be shipped after https://github.com/tc39/proposal-iterator-helpers/ ships. That proposal defines Iterator as a global object.

If an object is available by syntax, it is also a global object, for example, the Iterator.prototype is a global object (via Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))).

breakClosure is ...

I reviewed your polyfill again and I need to point out, that your polyfill has a very serious bug. Try this code:

const x = Iterator.range(10)
const y = Iterator.range(10)
console.log(x.next().value) // 9
console.log(x.next().value) // 8
console.log(y.next().value) // 7 ?!
console.log(y.next().value) // 6 ?!

You store the value on the Iterator global object, instead of each iterator, so @ljharb and @tabatkins are saying you're modifying the global state. The counter in your polyfill becomes a global state. (It also met my standard of global state, "information can be shared between two different programs that don't have direct contact" by modifying Iterator.breakClosure).

I know that seeing this all over the place automatically creates a bias but,

We won't. JavaScript spec has this everywhere.

please explain why the extra measure of offering an observable difference between the polyfill this repo is for

const generator = (function *() {}())
const generatorNext = generator.next
const arrayIterator = [].values()
const arrayIteratorNext = arrayIterator.next

// TypeError: next method called on incompatible Array Iterator
// generatorNext.call(arrayIterator)

generatorNext.call(generator) // ok

// TypeError: next method called on incompatible Generator
// arrayIteratorNext.call(generator)

arrayIteratorNext.call(arrayIterator) // ok

next method has a "brand", you can only call next with the corresponding object. My polyfill also simulates this behavior, therefore I need to change the %GeneratorPrototype%.next.

@ogbotemi-2000
Copy link
Author

I reviewed your polyfill again and I need to point out, that your polyfill has a very serious bug. Try this code:

I am aware of this behavior and I explained why that is so here: #63 (comment), the last code block in the comments offered code examples and use cases pertaining to your concern. It is not a bug, it was planned and it makes for an interesting behavior in the case of an unending iteration - you'd want only new Iterator.range(...args) calls to give a new iterator, while having the existing ones stored.

I wasn't aware that Iterator will acquire global cred further down the line, perhaps you could prepare a readily-available, short list that states the future of this proposal in order to make "contributors" write code with its evolution in mind.

I agree totally with your code above though, speed is not as important as unanimity of code, I guess.

I had originally sent my first code suggestions for internal implementations only: #63 (comment) hence why I used an unconventional API code style.

I look forward to Iterator.range being shipped soon, is there anyway I can help with its actual progress in the meantime?

@ljharb
Copy link
Member

ljharb commented Sep 11, 2023

How else would someone access a builtin X.y if X isn’t a global?

@Jack-Works
Copy link
Member

See tc39/proposal-iterator-helpers#286, they have some problems now. If they're going to rename, we'll also follow.

@ogbotemi-2000
Copy link
Author

@ljharb You are right. It was this phrase in @tabatkins comment '...global state...' that confused me and didn't realize that you all meant window.Iterator when you said global

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

No branches or pull requests

5 participants