Skip to content

fixed: Missing folder structure in html output #50 - #65

Merged
jdonaghue merged 4 commits into
SitePen:masterfrom
devCrossNet:master
Jul 18, 2016
Merged

jdonaghue merged 4 commits into
SitePen:masterfrom
devCrossNet:master

Conversation

@devCrossNet

Copy link
Copy Markdown
Contributor

I hope the changes will solve the issue: Missing folder structure in html output #50

@codecov-io

codecov-io commented Jun 2, 2016 •

Copy link
Copy Markdown

Current coverage is 99.25%

Merging #65 into master will increase coverage by 1.78%

@@             master        #65   diff @@
==========================================
  Files             9          9          
  Lines           395        402     +7   
  Methods           0          0          
  Messages          0          0          
  Branches         15         15          
==========================================
+ Hits            385        399    +14   
+ Misses            5          3     -2   
+ Partials          5          0     -5   

Powered by Codecov. Last updated by 659f449...041fb1e

@devCrossNet

Copy link
Copy Markdown
Contributor Author

@kitsonk is it possible to merge this pull request in the next days? or is there a reason why it takes so long?

@bryanforbes

Copy link
Copy Markdown
Member

I just tried the patch with a webpack bundle of several unit tests plus supporting code and it generates an invalid fileName:

origSourceFilename: webpack:///webpack/bootstrap de2b4ff8ec450d37b9d3?60be
origFileName: unit.bundle.js
fileName: /path/to/dist.test/unit.bundle

It then throws an error when originalPositionFor() is called at line 76 of remap.js.

@devCrossNet

Copy link
Copy Markdown
Contributor Author

hey @bryanforbes, can you provide an example of your setup? does remap work as expected without my patch or do you also have the issue #50 ?

@delasteve

Copy link
Copy Markdown

Could this be related?

#68

@devCrossNet

Copy link
Copy Markdown
Contributor Author

@delasteve @bryanforbes the problem was a missing file extension. I fixed that bug but I did not fix #68 . But I have a working setup to fix #68 . Maybe I will make another PR if I have enough time this weekend.

@Blackbaud-PaulCrowder

Copy link
Copy Markdown

I'd like to see this merged in, too, since we're currently referencing @devCrossNet's fork.

Comment thread lib/remap.js Outdated
/* if there are inline sources and a store to put them into, we will populate it */
var inlineSourceMap = {};
if (sourceMap.sourcesContent) {
var origSourceFilename = rawSourceMap.sources[0],

@dinoboff dinoboff Jul 7, 2016 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You should declare the variables outside of the if block for consistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll change that asap

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

@delasteve

Copy link
Copy Markdown

@bryanforbes Any chance you could take a look at this again?

@jdonaghue
jdonaghue merged commit 58e0e69 into SitePen:master Jul 18, 2016
@jdonaghue

Copy link
Copy Markdown
Contributor

@devCrossNet Thanks for your work here, looks great!

@devCrossNet

Copy link
Copy Markdown
Contributor Author

you're welcome

@devCrossNet

Copy link
Copy Markdown
Contributor Author

@jdonaghue can you tell me in which version this fix will be released? and when? regards

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.

7 participants