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

feat!: support source map with multiple sources #102

Merged
merged 8 commits into from
Aug 2, 2020

Conversation

MoLow
Copy link
Contributor

@MoLow MoLow commented Jun 26, 2020

No description provided.

@coveralls
Copy link

coveralls commented Jun 26, 2020

Pull Request Test Coverage Report for Build 444

  • 64 of 64 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 96.204%

Totals Coverage Status
Change from base Build 440: 0.7%
Covered Lines: 539
Relevant Lines: 556

💛 - Coveralls

@MoLow
Copy link
Contributor Author

MoLow commented Jun 26, 2020

I have seen #91 after implementing this.
my soultion seemd a little cleaner, I have compared results of runs and they look the same

@MoLow
Copy link
Contributor Author

MoLow commented Jun 28, 2020

@bcoe @lukeapage wdyt?

@benjamingr
Copy link

@bcoe any chance this could get a review ?

@bcoe
Copy link
Member

bcoe commented Jul 8, 2020

@MoLow thank you for taking a crack at this, I'll make an effort to give you a more thorough review soon.

},
},
"b": Object {},
"branchMap": Object {},
Copy link
Member

Choose a reason for hiding this comment

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

this seems weird to me, I wouldn't have expected us to need to update the snapshot for the non-multi-source use-case, and it looks like this updates the snapshot with empty information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bcoe this was indeed a bug, I fixed it

@bcoe
Copy link
Member

bcoe commented Aug 1, 2020

@MoLow sorry for the slow review, this implementation seems close to what I was expecting we'd do 👍

Great work, and thank you @LukePage for getting this in motion.

@bcoe
Copy link
Member

bcoe commented Aug 1, 2020

Hey @MoLow I've been working on dual mode modules for yargs-parser, so actually better understand the problem we're trying to solve here and could give better review.

Issues with yargs-parser

I tried running c8 against yargs-parser with your branch of v8-to-istanbul. The results for text output are very promising:

Screen Shot 2020-08-01 at 12 31 44 PM

However, when I open an HTML report there are some issues:

Screen Shot 2020-08-01 at 12 33 46 PM

My guess is we're missing resolving a path somewhere?

Issues with c8 test suite

I also ran the test suite against c8 and am getting a couple additional path related failures:

Screen Shot 2020-08-01 at 12 35 03 PM


I bet there's a chance these issues are related, and we're just missing a resolve somewhere.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Left some feedback @MoLow 👋 it seems like this work is a good chunk of the way there, just a couple of path related bugs we need to hunt down.

@bcoe bcoe changed the title support source map with multiple sources feat: support source map with multiple sources Aug 1, 2020
@bcoe
Copy link
Member

bcoe commented Aug 2, 2020

@MoLow update, I got everything debugged; your work was 99% of the way there 😄

@bcoe bcoe changed the title feat: support source map with multiple sources feat!: support source map with multiple sources Aug 2, 2020
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

awesome work @MoLow 🎉

Thank you for seeing this feature over the finish line. And thank you @lukeapage for kicking of this work.

@bcoe bcoe merged commit d1f435c into istanbuljs:master Aug 2, 2020
bcoe added a commit to bcoe/c8 that referenced this pull request Aug 3, 2020
Adds support for 1:many source-maps. These source maps are used by tools
like rollup or WebPack to bundle multiple JavaScript source files into one.

Refs: istanbuljs/v8-to-istanbul#102
bcoe added a commit to bcoe/c8 that referenced this pull request Aug 3, 2020
Adds support for 1:many source-maps. These source maps are used by tools
like rollup or WebPack to bundle multiple JavaScript source files into one.

Refs: istanbuljs/v8-to-istanbul#102
artempub added a commit to artempub/ceight that referenced this pull request Jan 17, 2023
Adds support for 1:many source-maps. These source maps are used by tools
like rollup or WebPack to bundle multiple JavaScript source files into one.

Refs: istanbuljs/v8-to-istanbul#102
mcknasty pushed a commit to mcknasty/c8 that referenced this pull request Feb 2, 2024
Adds support for 1:many source-maps. These source maps are used by tools
like rollup or WebPack to bundle multiple JavaScript source files into one.

Refs: istanbuljs/v8-to-istanbul#102
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.

4 participants