Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jun 11, 2018

Discovered #24866.

@ghost ghost force-pushed the testGotoDefWithProjectReferences branch from 31baae1 to a57712c Compare June 15, 2018 18:45
@ghost ghost requested a review from weswigham June 15, 2018 18:46
@ghost ghost force-pushed the testGotoDefWithProjectReferences branch from a57712c to 9a6512f Compare June 15, 2018 21:31
@ghost
Copy link
Author

ghost commented Jun 23, 2018

@weswigham The test is still wrong, could you take a look?

@ghost ghost force-pushed the testGotoDefWithProjectReferences branch from d7bbbd0 to b9ed8d6 Compare June 25, 2018 18:55
@ghost ghost force-pushed the testGotoDefWithProjectReferences branch from b9ed8d6 to 7c17aeb Compare June 25, 2018 19:44

writeDeclarationFiles(aTs, host, session, [
// Need to mangle the sourceMappingURL part or it breaks the build
{ name: "/a/bin/a.d.ts.map", text: '{"version":3,"file":"a.d.ts","sourceRoot":"","sources":["../a.ts"],"names":[],"mappings":"AAAA,kCAAsB"}' },
Copy link
Member

@weswigham weswigham Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sourcemap maps position 0 in the .d.ts to 0 in the .ts and 34 in the .d.ts to 22 in the .ts. The start of the declaration is mapped to the start, and the end is mapped to the end, nothing more. We try to map position 24, which is squarely between those, so return the closest available mapping - 22, the end mapping. toFileSpan in the LS then maps that into a 1-based line & column system. The end position is out of bounds, because, ofc, there's no 1-character identifier at the end of the declaration (and we assume that the length of the declaration span is constant when we map it).

@ghost ghost requested a review from mhegazy June 27, 2018 00:50
@ghost ghost force-pushed the testGotoDefWithProjectReferences branch from 49d656d to 0dca8f1 Compare June 27, 2018 00:50
@ghost
Copy link
Author

ghost commented Jun 27, 2018

@weswigham @mhegazy Good to merge?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests. 🐱

const navtoResponse = session.executeCommand(navtoRequest).response as protocol.NavtoResponse["body"];

assert.deepEqual(navtoResponse, [
// TODO: First result should be from a.ts, not a.d.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a reminder because navTo doesn't apply sourcemaps yet, right?

Copy link
Author

@ghost ghost Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, working on it. #25283

@ghost ghost merged commit 16f045b into master Jun 27, 2018
@ghost ghost deleted the testGotoDefWithProjectReferences branch June 27, 2018 22:56
This pull request was closed.
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.

1 participant