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

Added source File name for emitAsset func #14603

Closed
wants to merge 3 commits into from

Conversation

g4rry420
Copy link

Signed-off-by: Gurkiran Singh gurkiransinghk@gmail.com

What kind of change does this PR introduce?
Added the source File Info for emitAsset func .
Resolves #13695
Since, this is my first pr to webpack, I wasn't sure how to test this after making the changes. If someone can please let me know, that be really helpful.

Did you add tests for your changes?
No

Does this PR introduce a breaking change?
No

What needs to be documented once your changes are merged?
Nothing

Signed-off-by: Gurkiran Singh <gurkiransinghk@gmail.com>
@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@@ -4086,7 +4086,7 @@ This prevents using hashes of each other and should be avoided.`);
if (!isSourceEqual(this.assets[file], source)) {
this.errors.push(
new WebpackError(
`Conflict: Multiple assets emit different content to the same filename ${file}`
`Conflict: Multiple assets emit different content to the same filename ${file}. Source File: ${source}`
Copy link
Member

Choose a reason for hiding this comment

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

source is the content of the file.

You could include assetInfo in the message instead, this might give hints.

Signed-off-by: Gurkiran Singh <gurkiransinghk@gmail.com>
@webpack-bot
Copy link
Contributor

@g4rry420 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@@ -4086,7 +4086,7 @@ This prevents using hashes of each other and should be avoided.`);
if (!isSourceEqual(this.assets[file], source)) {
this.errors.push(
new WebpackError(
`Conflict: Multiple assets emit different content to the same filename ${file}`
`Conflict: Multiple assets emit different content to the same filename ${file}. Source File: ${assetInfo}`
Copy link
Member

Choose a reason for hiding this comment

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

You can't print an object that way, you might want to JSON.stringify it, or print it in a nicer way.

Anyway, please also make sure to test the code. e. g. here is a test case you can update: test/configCases/emit-asset/different-source

Copy link
Author

Choose a reason for hiding this comment

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

My apologies.
For testing, I have run the yarn jest ConfigTestCases and from my understanding the testing has been done in ConfigTestCases.tempate.js file which grabs the files from configCases folder and tests them. But still didn't understand what should I update in test/configCases/emit-asset/different-source ? Please, let me know about it.

Signed-off-by: Gurkiran Singh <gurkiransinghk@gmail.com>
@alexander-akait
Copy link
Member

/cc @snitin315 Can you resend this and add a test case, thank you

@alexander-akait
Copy link
Member

Fixed by #15888

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve emitAsset error with sourceFilename
4 participants