This type should be defined above:

let trackedBindings: TrackedBindings

done

we should use const instead of let when the variable doesnt get reassigned. It makes it easier to read the code, otherwise I end up searching for where trackBindings is reassigned.

It looks like this can be changed for all instances of let in this block of code. There is no reassignment of the objects, only modification of their properties.

done

from line 231 - 241 -- we should factor this out into a function

function isExactPosition(originalPosition, position) {
  const originalPositionTest = map.originalPositionFor({
    line: position.line,
    column: position.column,
    bias: SourceMapConsumer.LEAST_UPPER_BOUND
  });
  return (
    !originalPositionTest ||
    originalPositionTest.name !== originalPosition.name ||
    originalPositionTest.column !== originalPosition.column ||
    originalPositionTest.line !== originalPosition.line
  );
}

and do if (!isExactPosition(originalPosition, position) here

it would be good to save this boolean in a variable with a descriptive name; or we can extract this into a function

can you tell me a bit about what you put into this loop, and what should be the result when this loop is finished? I imagine at the moment that we have the following signature

Array<scopeBindingName> -> Map<name, validBestLocation>

and discarding cases of a scope name where there is no bestLocation

if this is the case, a reduce would fit better here; it would be more explicit what is happening

I suspect the same thing is happening here, where we have the following signature
Array<Position> -> Map<name, bestLocation>

And a reduce might be more explicit; we could have something like this:

const bestLocations = positions.reduce(findBestLocations(name))

and findBestLocation does (name) -> (bindings, Position) -> Map<name, bestLocation>

positions.reduce might need the initial value positions.reduce(position => findBestLocations(position), {})

find best location shall be performed across all generated names -- I reviewed summary comment before algorithm and provided example why things are done the way they are.

this should be a reduce as well

fixed here and above.

I think getLocationScope is sufficiently complex to justify its own module with some helper methods.

function appendTrackedBinding() {}

function appendBinding() {}

function getScopeBindings() {}

function getLocationScope() {

  generatedSourceScopes.map(scope => {
    // ...
    const trackedBindings = bindings.reduce(appendTrackedBinding, {})
    const bindings = trackedBindings.reduce(appendBinding, {});
    return {
      type: scope.type,
      bindings,
    };
  })
}

module.exports = { getLocationScope}

appendTrackedBinding and appendBinding extraction is not possible due to dependency on location/currentPosition.

getLocationScope also depends on _getSourceMap

we might need to do a bit more splitting to share _getSourceMap between modules.

we also might need to pass in the closure state:

bindings.reduce(binding => appendTrackedBinding(binding, location, ...), {})

If this feels like too much we can punt on it.

Did not extracted _getSourceMap -- just exposed it from ./source-map

Moved scopes into separate file and refactored to have appendTrackedBinding.

I'd like jsdoc for all new exported functions. Lack of documentation is one of the bigger problems in this entire module already.

we can clean this up after we add the parser code and can improve the test in one pass.