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

Try 2 bias values when calling originalPositionFor from applySourceMap #376

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MotorHeat
Copy link

The method applySourceMap is calling originalPositionFor without providing the bias value.
By default bias equals to GREATEST_LOWER_BOUND which means: if exact column value was not found - take nearest item from the left. In some cases there is "no nearest value on the left" because most correct value is nearest "from the right". I believe it is worth to try to call originalPositionFor with both available bias values rather then just try one and claim that we cannot find mapping while it is exists.

Initially the GREATEST_LOWER_BOUND value is used (like it was before). If originalPositionFor cannot find relevant mapping then LEAST_UPPER_BOUND bias value is used to search again.
I don't think that applySourceMap should made any assumptions on the structure of the source maps and it should be symmetric as much as possible (in proposed implementation preference is given to the "left" element but this is how it always was before).

@coveralls
Copy link

coveralls commented Jan 20, 2019

Pull Request Test Coverage Report for Build 543

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 88.44%

Totals Coverage Status
Change from base Build 539: 0.06%
Covered Lines: 829
Relevant Lines: 916

💛 - Coveralls

@jasonLaster
Copy link

I think this is a nice idea. I'm curious what others will think

@bcoe
Copy link

bcoe commented Apr 22, 2019

@jasonLaster @MotorHeat rather than trying both LEAST_UPPER_BOUND and GREATEST_LOWER_BOUND, might it instead make sense to return the mid point during the binary search process rather than failure? I'm not sure how to read the WASM version of the module, but looking at the older version of the library, it looks like the search logic is something along these lines:

    if (aBias == exports.LEAST_UPPER_BOUND) {
      return aHigh < aHaystack.length ? aHigh : -1;
    } else {
      return mid;
    }

Where if aHigh, as an example, is greater than our haystack we return a failure, rather than simply returning the closest element mid.

Am I missing something, or would it be good to have a mode that's more resilient like this?

@ochameau
Copy link
Contributor

ochameau commented Oct 4, 2022

I'm not sure how to read the WASM version of the module,
The WASM code is here:
https://github.com/fitzgen/source-map-mappings/blob/197660faf3dd1167c643797a5ed62cb687482c54/src/lib.rs#L346-L356

Hi @MotorHeat @bcoe, thanks for the patch.

I'm trying to revive the maintenance of this package....
Would you have any example where this code fails?
It would help write a test to cover this and better understand if we should go for the mid point or the left or right search.

@bcoe
Copy link

bcoe commented Oct 5, 2022

@ochameau hey 👋, sorry I've paged this problem out of my brain 😆

I think it related to this code I was writing a few years ago, to try to make a best effort guess at what line to remap to.

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 this pull request may close these issues.

5 participants