Implementation bug in normaliseInput

[Gozala]

I am pretty sure following lines would throw TypeError: iterator is not iterable because yield * expression expects iterable and not an iterator.

js-ipfs/packages/ipfs-core-utils/src/files/normalise-input.js

Lines 73 to 82 in 8cb8c73

	const iterator = input[Symbol.iterator]()
	const first = iterator.next()
	if (first.done) return iterator
	// Iterable<Number>
	// Iterable<Bytes>
	if (Number.isInteger(first.value) || isBytes(first.value)) {
	yield toFileObject((function * () {
	yield first.value
	yield * iterator

js-ipfs/packages/ipfs-core-utils/src/files/normalise-input.js

Lines 114 to 122 in 8cb8c73

	const iterator = input[Symbol.asyncIterator]()
	const first = await iterator.next()
	if (first.done) return iterator
	// AsyncIterable<Bytes>
	if (isBytes(first.value)) {
	yield toFileObject((async function * () { // eslint-disable-line require-await
	yield first.value
	yield * iterator

js-ipfs/packages/ipfs-core-utils/src/files/normalise-input.js

Lines 191 to 199 in 8cb8c73

	const iterator = input[Symbol.iterator]()
	const first = iterator.next()
	if (first.done) return iterator
	// Iterable<Number>
	if (Number.isInteger(first.value)) {
	yield toBuffer(Array.from((function * () {
	yield first.value
	yield * iterator

It is also worrying that no tests seem to catch that.

[Gozala]

Also it appears that parenthesis is in the wrong place here

js-ipfs/packages/ipfs-core-utils/src/files/normalise-input.js

Line 52 in 8cb8c73

throw errCode(new Error(`Unexpected input: ${input}`, 'ERR_UNEXPECTED_INPUT'))

[achingbrain]

Please can you submit a PR with a failing test, and preferably, a fix.

[Gozala]

I'm working on the patch that adds tests and fixes.

[Gozala]

Here is the example that illustrates the issue:

class IterableOf {
  constructor(...entries) {
    this.entries = entries
  }
  [Symbol.iterator]() {
    return new IteratorOf(...this.entries)
  }
}

class IteratorOf {
  constructor(...entries) {
    this.position = 0
    this.entries = entries
  }
  next() {
    if (this.position < this.entries.length) {
      return { done: false, value: this.entries[this.position++] }
    } else {
      return { done: true }
    }
  }
}

function * test () {
  const iterable = new IterableOf(1, 2, 3)
  const iterator = iterable[Symbol.iterator]()
  
  yield iterator.next().value
  yield * iterator
}

[...test()] // Uncaught TypeError: iterator is not iterable

I should note that browser built-ins (including generators) tend to return iterators that effectively have [Symbol.iterator]() { return this } and there for this doesn't appear like a problem. That said iterables are not required to return iterators are also iterables.

[achingbrain]

I don't think you can use yield * like that. If I run your example I get:

  yield * iterator
  ^

TypeError: undefined is not a function

If I change it to a for..of I get:

  for (const thing of iterator) {
                      ^

TypeError: iterator is not iterable

Which is true, it's not iterable. Something is iterable if it has a [Symbol.iterator] function that returns an iterator which the value of iterator does not.

[achingbrain]

Another problem is the iterators can have state, you can't just yield * the iterable:

function * test () {
  const iterable = new IterableOf(1, 2, 3)
  const iterator = iterable[Symbol.iterator]()

  yield iterator.next().value
  yield * iterable
}

console.info([...test()])
// [ 1, 1, 2, 3 ]

If you drop down to the low-level interface you can avoid it, which I guess is the solution here:

function * test () {
  const iterable = new IterableOf(1, 2, 3)
  const iterator = iterable[Symbol.iterator]()

  yield iterator.next().value

  while (true) {
    const { value, done } = iterator.next()

    if (done) {
      break
    }

    yield value
  }
}

console.info([...test()])
// [ 1, 2, 3 ]

[Gozala]

I don't think you can use yield * like that. If I run your example I get:

Yes that was what I was trying to illustrate. You see a different error because I'm guessing you ran it on V8 while I've run it on SpiderMonkey. They produce different error messages but result is the same.

[Gozala]

Another problem is the iterators can have state, you can't just yield * the iterable:

That is why I introduced Peekables in #3151, so you could peek at first item, without consuming it & also use the whole thing as iterable.

js-ipfs/packages/ipfs-core-utils/src/files/normalise-input.js

Lines 409 to 452 in dcedb66

	/**
	* @template T
	*/
	class Peekable {
	/**
	* @template T
	* @template {Iterable<T>} I
	* @param {I} iterable
	* @returns {Peekable<T>}
	*/
	static from (iterable) {
	return new Peekable(iterable)
	}
	/**
	* @private
	* @param {Iterable<T>} iterable
	*/
	constructor (iterable) {
	const iterator = iterable[Symbol.iterator]()
	/** @private */
	this.first = iterator.next()
	/** @private */
	this.rest = iterator
	}
	peek () {
	return this.first
	}
	next () {
	const { first, rest } = this
	this.first = rest.next()
	return first
	}
	[Symbol.iterator] () {
	return this
	}
	[Symbol.asyncIterator] () {
	return this
	}
	}

[achingbrain]

That is why I introduced Peekables in #3151

Yes, it was a good idea - I broke some similar code out into it-peekable