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

fix: resolve the inconsistent type issue in JsModule #8939

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shulaoda
Copy link
Collaborator

@shulaoda shulaoda commented Jan 6, 2025

Summary

closes #8937

Checklist

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

@shulaoda shulaoda requested a review from SyMind January 6, 2025 03:51
@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Jan 6, 2025
Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 026052f
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/677b58da3bb05400086417dd

Copy link

codspeed-hq bot commented Jan 6, 2025

CodSpeed Performance Report

Merging #8939 will not alter performance

Comparing shulaoda:fix/type-js-module (026052f) with main (f81f1c7)

Summary

✅ 3 untouched benchmarks

@shulaoda shulaoda marked this pull request as draft January 6, 2025 04:10
@shulaoda shulaoda marked this pull request as ready for review January 6, 2025 04:25
SoonIter
SoonIter previously approved these changes Jan 7, 2025
set(val: string) {
module.userRequest = val;
set(val: string | undefined) {
if (val) {
Copy link
Member

Choose a reason for hiding this comment

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

what if val is an empty string ''?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The userRequest property exists only when the module is a NormalModule; it does not exist in other cases. When the userRequest of a NormalModule is an empty string (''), it might overlap in meaning with cases where the userRequest is absent in non-NormalModule instances. I am uncertain whether this change is appropriate.

@SoonIter SoonIter dismissed their stale review January 7, 2025 06:04

comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: bug fix release: bug related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: TS Errors When Running With skipLibCheck: false
2 participants