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

Add support for file source maps #76

Closed

Conversation

aaronjensen
Copy link
Collaborator

Addresses #51

@aaronjensen
Copy link
Collaborator Author

This currently only applies to when optionsCount <= 1 (I'm not really sure what that is...) but I can adapt it to the other if this looks good.

@aaronjensen
Copy link
Collaborator Author

Ah... it looks like it's for supporting multiple webpack configs. It concatenates all of the output files... I'm not sure how that could work with sourcemaps. It probably didn't w/ the old method, did it?

We'd have to fetch all of the source maps and adjust the line numbers to deal w/ the concat, right?

@aaronjensen
Copy link
Collaborator Author

For anyone coming here, I published https://www.npmjs.com/package/karma-webpack-with-fast-source-maps for now until this is updated

@dignifiedquire
Copy link
Collaborator

This looks really nice to me. Thanks @aaronjensen
@sokra could you take a look when you have a moment please, and let me know if I'm okay to merge this.

@sokra
Copy link
Contributor

sokra commented Aug 20, 2015

It would be great if this also work for the optionsCount > 1 case. Shouldn't be to complex to adjust it.

@aaronjensen
Copy link
Collaborator Author

@sokra @dignifiedquire Thanks.

I did not really know how to test with optionsCount > 1. Am I right that we would have to concat all of the source maps and adjust line numbers?

Does anyone know why cheap-module-inline-source-map does not work w/ the current version?

@sokra
Copy link
Contributor

sokra commented Aug 20, 2015

Am I right that we would have to concat all of the source maps and adjust line numbers?

Yes, but there are tools out there which do this for you. You can use the source-map lib, which is the lib devtool: "source-map" uses, or you can use the source-list-map lib, which is the lib devtool: "cheap-source-map" and variants uses.

source-map: 
SourceNode.fromStringWithSourceMap
  -> new SourceNode(..., [a, b, c])
  -> SourceNode.toStringWithSourceMap.

source-list-map:
fromStringWithSourceMap(...)
  -> SourceListMap.add
  -> SourceListMap.toStringWithSourceMap

@aaronjensen
Copy link
Collaborator Author

fyi @sokra I don't know how to progress on this because of #77

I also don't know what multiple configurations is used for. Thanks!

@aaronjensen aaronjensen mentioned this pull request Oct 11, 2015
@joshwiens
Copy link
Contributor

@aaronjensen - What do you want to do here? We can resurrect this PR after we merge in all the code validations / es6 support or get the team to brainstorm a solution. Sourcemaps / Performance are on the hotlist once we have proper testing in place.

@aaronjensen
Copy link
Collaborator Author

@d3viant0ne Yes, that'd be great. I didn't feel comfortable with this change given the lack of coverage and my lack of understanding of multiple webpack configs, but it'd be great to get something like this merged in so my fork can go away.

@jsf-clabot
Copy link

jsf-clabot commented Jan 19, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@joshwiens
Copy link
Contributor

That was unintentionally closed when I fixed the master branch. @aaronjensen next time you have 3/4 minutes free, ping me in slack.

@aaronjensen
Copy link
Collaborator Author

@d3viant0ne i pinged you on gitter, not sure if that's what you meant or if there's a webpack slack I don't know about

@joshwiens joshwiens self-assigned this Mar 11, 2017
@joshwiens joshwiens closed this Mar 20, 2017
@mgol
Copy link

mgol commented Mar 20, 2017

@d3viant0ne Why did you close it? I don't see a fix on master.

@joshwiens
Copy link
Contributor

Fixing the master branch. This feature hasn't gone anywhere nor has the plan changed, @aaronjensen is a part of the webpack organization. This effort will be taken up again at the appropriate time.

@mgol
Copy link

mgol commented Mar 20, 2017

OK, thanks for the update.

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.

None yet

6 participants