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

[Feature]: support experimental best-effort source-map remapping in module errors #5632

Open
Tracked by #3459
h-a-n-a opened this issue Feb 7, 2024 · 8 comments
Open
Tracked by #3459
Labels
contrib: medium feat New feature or request pr welcome stale team The issue/pr is created by the member of Rspack.

Comments

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Feb 7, 2024

What problem does this feature solve?

In the current implementation of rspack, module build error and module parse error would be mapped based on the output of loader, which leads its corresponding error span not being able to mark the user input.

What does the proposed API of configuration look like?

This will work better, in the best case scenario, if options.devtool set to module related options (for example: cheap-module-source-map) and will be remapped along with errors lazily for better performance.

This does not expose any JS APIs, however it could be enabled with experiments options, the exact option name remains to be determined. This is built-on the contract that source-map must be correct, otherwise the remapped source or span message might be incorrect. So this feature should be opt-in in compiler-wise or module rule wise.

@h-a-n-a h-a-n-a added feat New feature or request pending triage The issue/PR is currently untouched. and removed pending triage The issue/PR is currently untouched. labels Feb 7, 2024
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Feb 7, 2024
@h-a-n-a
Copy link
Collaborator Author

h-a-n-a commented Feb 7, 2024

Compiler-wise

Toggle made to the whole compiler, turning in on means this will apply to every module during each compilation.

Pro:

  1. Easy to use

Con:

  1. The correctness of sourcemap should be guaranteed, otherwise the result will be not correct.

Module rule wise

Support toggling in module rule wise, this can only opt-in only a certain pieces of modules in the remapping, for example: user code.

Pro:

  1. Flexible enough to turn off for certain modules.
  2. Potential benchmark gains, as modules in node_modules often does necessarily to be mapped.

Con:

  1. Place to put this option remains to be determined.

@hardfist
Copy link
Contributor

hardfist commented Feb 7, 2024

I think this should be a opt-in feature(other than opt-out) considering we have no way to guarantee the source-map correctness and it's the same behavior like --enable-source-maps.
From my personal experience, incorrect node_modules source-map is very normal(people may forget to upload source file to npm but keeps source file in source-map)
maybe we can implement it as a user land plugin first and see whether it works for most scenarios

@h-a-n-a
Copy link
Collaborator Author

h-a-n-a commented Feb 7, 2024

I think this should be a opt-in feature(other than opt-out) considering we have no way to guarantee the source-map correctness and it's the same behavior like --enable-source-maps. maybe we can implement it as a userland plugin first and see whether it works for most scenarios

I agree with you, and I would suggestion this feature should be opt-in compiler-wise or module rule wise. But feel free to propose with a better solution if you have any ;-).

Edit: Plugin looks good to me too, but it does not solve the problem of placing it on compiler-wise or module rule wise.

From my personal experience, incorrect node_modules source-map is very normal(people may forget to upload source file to npm but keeps source file in source-map)

In this case, if there's no either sourceContent or file available in the source-map, then rspack should bail-out.

@hardfist
Copy link
Contributor

hardfist commented Feb 7, 2024

Edit: Plugin looks good to me too, but it does not solve the problem of compiler-wise or module rule wise.

It seems you can control this in plugin options? since you got whole sourcemap and error location info in stats, so you can do reverse mapping for any module?

@h-a-n-a
Copy link
Collaborator Author

h-a-n-a commented Feb 7, 2024

Edit: Plugin looks good to me too, but it does not solve the problem of compiler-wise or module rule wise.

It seems you can control this in plugin options? since you got whole sourcemap and error location info in stats, so you can do reverse mapping for any module?

Sure we have the capability to handle this, the design part should be deliberately considered for the best user experience.

Copy link

stale bot commented May 21, 2024

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the stale label May 21, 2024
@h-a-n-a
Copy link
Collaborator Author

h-a-n-a commented Jun 6, 2024

bump

@stale stale bot removed the stale label Jun 6, 2024
Copy link

stale bot commented Aug 5, 2024

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the stale label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contrib: medium feat New feature or request pr welcome stale team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

No branches or pull requests

2 participants