Skip to content

Assume sourcemap name if blank#674

Closed
pgross41 wants to merge 1 commit intowuct:masterfrom
pgross41:default-sourcemap-name
Closed

Assume sourcemap name if blank#674
pgross41 wants to merge 1 commit intowuct:masterfrom
pgross41:default-sourcemap-name

Conversation

@pgross41
Copy link
Copy Markdown

@pgross41 pgross41 commented Apr 22, 2021

I was using this successfully on webpack 3 but it fails after upgrading to webpack 5 with the following error, written multiple times:

⬡ ElasticAPMSourceMapPlugin: there is no .js files to be uploaded.

I determined the sourceFile was populated but the sourceMap was blank. I'm sure there is a correct way to do this with webpack 5 but this is a nice cheap solution. 🙂

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #674 (7f213c7) into master (92e5d02) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #674   +/-   ##
=======================================
  Coverage   96.55%   96.55%           
=======================================
  Files           1        1           
  Lines          29       29           
  Branches        4        4           
=======================================
  Hits           28       28           
  Partials        1        1           
Impacted Files Coverage Δ
src/elastic-apm-sourcemap-webpack-plugin.ts 96.55% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92e5d02...7f213c7. Read the comment docs.

@pgross41
Copy link
Copy Markdown
Author

@wuct please review

@wuct
Copy link
Copy Markdown
Owner

wuct commented Apr 28, 2021

@pgross41 the current code assume js source map files have file extentions .js.map here. You can control how webpack generates the filename of source map by configuring output.sourceMapFilename. However, I think it is useful that ElasticAPMSourceMapPlugin can provide a config to enable sourcemap filename customization. I will happy to accept a diff for it. :)

@pgross41
Copy link
Copy Markdown
Author

@wuct my files are using .js.map but the sourceMap param is undefined. Added a breakpoint to the location you indicated. it looks like in webpack 5 the sourcemap files aren't in files they are in auxiliaryFiles
image

Would you prefer something like this over the code that is currently in the PR?

const allFiles = [...files, ...auxiliaryFiles];
const sourceFile = R.find(R.test(/\.js$/), allFiles);
const sourceMap = R.find(R.test(/\.js\.map$/), allFiles);

@wuct
Copy link
Copy Markdown
Owner

wuct commented Apr 30, 2021

@pgross41 ah, it makes sense to me! Do you mind to also update the webpack version here to 5 and check the integration test passed?

@pgross41
Copy link
Copy Markdown
Author

pgross41 commented May 3, 2021

@wuct I updated webpack to 5 but the integration test errors here with:

TypeError: Cannot read property 'source' of undefined

This expression is undefined:

compilation.assets[sourceMap]

I have tried tweaking it but don't even know what to change to get the file contents of the sourcemap. I also tried the allFiles idea above but auxiliaryFiles is empty. I can't figure out what is different about my build, it seems like my sourcemaps are coming from the TerserPlugin so I tried adding that but it had the same errors.

So it looks like this PR won't add any broad support for webpack 5 but it does add support for the particular webpack 5 config for our project 🙂

wuct pushed a commit that referenced this pull request May 3, 2021
@wuct
Copy link
Copy Markdown
Owner

wuct commented May 3, 2021

@pgross41 I've fixed the test in Webpack 5 in this branch. I also fixed the sourcemap finding issue in that branch. Could you help me test it? I am not using ElasticAPM recently. Thanks a lot!

@pgross41
Copy link
Copy Markdown
Author

pgross41 commented May 4, 2021

@wuct thanks for properly fixing my bandaid solution. I pulled the webpack5 branch, ran npm run build:js and plopped the js file into my project. It worked!

@pgross41
Copy link
Copy Markdown
Author

@wuct is your webpack5 branch ready for a release?

@wuct
Copy link
Copy Markdown
Owner

wuct commented May 18, 2021

@wuct is your webpack5 branch ready for a release?

Yes, I'll release it this week. Just too busy these days, sorry about that.

@pgross41
Copy link
Copy Markdown
Author

No problem, I totally understand and I appreciate your time!

wuct pushed a commit that referenced this pull request May 20, 2021
wuct pushed a commit that referenced this pull request May 20, 2021
wuct pushed a commit that referenced this pull request May 20, 2021
wuct pushed a commit that referenced this pull request May 21, 2021
wuct pushed a commit that referenced this pull request May 21, 2021
@wuct wuct closed this in #695 May 21, 2021
wuct pushed a commit that referenced this pull request May 21, 2021
## [1.6.2](v1.6.1...v1.6.2) (2021-05-21)

### Bug Fixes

* fix souremap files finding in webpack 5 ([070246f](070246f)), closes [#674](#674)
@pgross41 pgross41 deleted the default-sourcemap-name branch May 24, 2021 16:34
@pgross41
Copy link
Copy Markdown
Author

Thank you @wuct!

@wuct
Copy link
Copy Markdown
Owner

wuct commented May 26, 2021

@pgross41 you are very welcome! Thanks for bringing it up!

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.

3 participants