Uh oh!
There was an error while loading. Please reload this page.
Off by one(1) error in column addressing: fixing originalPositionFor() API for indexed maps + added test adapted from the 0.6.x dev tree - #399
Conversation
…ot` is checked everywhere using `!= null` except in 1(one) spot, which caused a crash in my test set, while debugging indexed map issues with the origin API.
…situation also throw an exception? In the other situation, where this API might also produce a NULL return, an exception *is* thrown instead, so this looks to me like the more consistent behaviour.
…ted from the 0.6.x dev tree: it turned out that due an off-by-1 error in the column value, half of the mapping nodes weren't hit by the `originalPositionFor()` internal search code, while it *should* find those. The added tests are augmented with extra logging for diagnosis: run those tests with & without the given patch to observe the bug & change.
this patch requires the fix provided in patch-3 to, at least partly, fix this error in the test bench: ``` FAILED ./test-source-map-consumer: test index map + originalPositionFor! TypeError: Cannot read property '0' of undefined at getURLType (\lib\js\source-map\lib\util.js:236:10) at relativeIfPossible (\lib\js\source-map\lib\util.js:373:19) at Object.relative (\lib\js\source-map\lib\util.js:366:18) at \lib\js\source-map\lib\source-map-generator.js:75:31 at Array.forEach (<anonymous>) at Function.fromSourceMap (\lib\js\source-map\lib\source-map-generator.js:72:32) at Object.exports.test index map + originalPositionFor (\lib\js\source-map\test\test-source-map-consumer.js:721:32) at <anonymous> ```
…rgument/option for the `consumer.sourceContentFor()` API.
…s code to restore intended behaviour. All tests pass.
This fix is required to make the tests pass. Without it, you'll see this failure/crash: ``` FAILED ./test-source-map-consumer: test index map + originalPositionFor! Error: "foo.js" is not in the SourceMap. at IndexedSourceMapConsumer.sourceContentFor (\lib\js\source-map\lib\source-map-consumer.js:839:11) at \lib\js\source-map\lib\source-map-generator.js:82:42 at Array.forEach (<anonymous>) at Function.fromSourceMap (\lib\js\source-map\lib\source-map-generator.js:72:32) at Object.exports.test index map + originalPositionFor (\lib\js\source-map\test\test-source-map-consumer.js:721:32) at <anonymous> ```
Pull Request Test Coverage Report for Build 561
💛 - Coveralls |
GerHobbelt
commented
Jun 18, 2019
Ha! This is a very close simile of #382: I browsed the pullreq list just now and discovered I had ripped that one's unit test into my 0.6.x and then went for 0.8.x and redid the work. However 😉 , the -1 approach is already present a little lower in the code in this little chunk that sits just below the place where we both patched the code: Also, this pullreq makes all tests pass and for that I had to mix in the other pullreqs mentioned earlier. Without them, the test kept failing; see also the console diagnostics in the commits. |
bomsy
commented
May 30, 2024
Closing as fixed by #507 |
The main work was done in SHA-1: 88efe61
originalPositionFor()API for indexed maps + added test adapted from the 0.6.x dev tree: it turned out that due an off-by-1 error in the column value, half of the mapping nodes weren't hit by theoriginalPositionFor()internal search code, while it should find those.The added tests are augmented with extra logging for diagnosis: run those tests with & without the given patch to observe the bug & change.
Before this pullreq, the 0.7.x/0.8.x did not test this situation; I ran into trouble with indexed maps crashing and acting crazy. The added test ensures there's no regression.
WARNING / NOTE: this pullreq includes #394 and #398 as required prerequisites: without those, this won't fly, as the crashes/failures in the test set will simply move to other spots in the code.
Hence this is more complex issue as #394 and #398 must be fixed before this one can work.
See the commit log for this one: patch-3 = #394 and patch-7 = #398 which are visible below as
git mergecommits.