Skip to content

fix: delay remove of intermediate debugging assets to afterEmit#251

Closed
upupming wants to merge 2 commits intolynx-family:mainfrom
upupming:fix/slardar
Closed

fix: delay remove of intermediate debugging assets to afterEmit#251
upupming wants to merge 2 commits intolynx-family:mainfrom
upupming:fix/slardar

Conversation

@upupming
Copy link
Copy Markdown
Collaborator

Summary

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@upupming upupming requested a review from colinaaa as a code owner March 19, 2025 12:13
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 19, 2025

🦋 Changeset detected

Latest commit: 0d965bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@lynx-js/template-webpack-plugin Patch
@lynx-js/web-webpack-plugin Patch
@lynx-js/react-rsbuild-plugin Patch
@lynx-js/react-alias-rsbuild-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 59.67742% with 25 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/webpack/web-webpack-plugin/src/index.ts 19.35% 25 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@colinaaa colinaaa left a comment

Choose a reason for hiding this comment

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

I don't believe this is the ideal way to resolve the issue. If you would like to consume the source-map in other plugin, maybe try to read source map from memory instead of expecting it to be generated on disk.

Comment thread packages/webpack/web-webpack-plugin/src/index.ts
files2Delete.push(asset + '.map');
}
for (const file of files2Delete) {
fs.rmSync(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Emitting the asset to disk first and then manually deleting it seems inefficient.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, actually the right way to consume our sourcemap is to read source map from memory using compiler.

But the plugin user used now are using fs to find all sourcemap files.

image

github-merge-queue bot pushed a commit that referenced this pull request Apr 7, 2025
<!--
  Thank you for submitting a pull request!

We appreciate the time and effort you have invested in making these
changes. Please ensure that you provide enough information to allow
others to review your pull request.

Upon submission, your pull request will be automatically assigned with
reviewers.

If you want to learn more about contributing to this project, please
visit:
https://github.com/lynx-family/lynx-stack/blob/main/CONTRIBUTING.md.
-->

## Summary

<!-- Can you explain the reasoning behind implementing this change? What
problem or issue does this pull request resolve? -->

This patch resolves a regression from issue #231 that prevented
obtaining the correct content hash for `background.js`. The fix involves
delaying the `processAssets` stage of Encode to
`PROCESS_ASSETS_STAGE_OPTIMIZE_HASH`.

The removal of debugging assets is postponed to
`compiler.hooks.afterEmit` to ensure accurate retrieval of assets and
their corresponding source maps.

close: #251

<!-- It would be helpful if you could provide any relevant context, such
as GitHub issues or related discussions. -->

## Checklist

<!--- Check and mark with an "x" -->

- [x] Tests updated (or not required).
- [ ] Documentation updated (or **not required**).
@colinaaa colinaaa closed this in #498 Apr 7, 2025
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.

2 participants