Skip to content

Comments

fix: convert severity of missing_export diagnostic to warning when importee module is ts/tsx#4147

Merged
IWANABETHATGUY merged 1 commit intomainfrom
04-17-fix_omit_missing_export_diagnostic_when_importee_module_is_ts_tsx
Apr 17, 2025
Merged

fix: convert severity of missing_export diagnostic to warning when importee module is ts/tsx#4147
IWANABETHATGUY merged 1 commit intomainfrom
04-17-fix_omit_missing_export_diagnostic_when_importee_module_is_ts_tsx

Conversation

@IWANABETHATGUY
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY commented Apr 17, 2025 •
edited
Loading

@IWANABETHATGUY Graphite App
Copy link
Member Author

IWANABETHATGUY commented Apr 17, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@netlify
Copy link

netlify bot commented Apr 17, 2025 •
edited
Loading

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 6ff517f
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/680098ca7dd23a000866a98f

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review April 17, 2025 04:50
@hyf0
Copy link
Member

hyf0 commented Apr 17, 2025

Would it be more straight to just enable shimMissingExports: true for #4144 (comment)?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2025 •
edited
Loading

Benchmarks Rust

  • target: main(bd7f736)
  • pr: 04-17-fix_omit_missing_export_diagnostic_when_importee_module_is_ts_tsx(6ff517f)
group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.04     74.7±1.05ms        ? ?/sec    1.00     71.7±0.94ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.01     94.8±0.96ms        ? ?/sec    1.00     93.9±1.23ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.00    108.2±1.64ms        ? ?/sec    1.00    108.6±2.15ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.01     83.9±1.31ms        ? ?/sec    1.00     83.3±2.48ms        ? ?/sec
bundle/bundle@rome-ts                                               1.03    124.6±2.05ms        ? ?/sec    1.00    121.4±1.00ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.00    201.0±3.02ms        ? ?/sec    1.06    212.7±6.67ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.02    244.9±3.73ms        ? ?/sec    1.00    240.7±3.22ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.00    137.0±2.53ms        ? ?/sec    1.00    137.5±1.49ms        ? ?/sec
bundle/bundle@threejs                                               1.00     42.2±0.57ms        ? ?/sec    1.00     42.2±1.72ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.02     87.5±3.21ms        ? ?/sec    1.00     86.0±1.00ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.00    102.2±0.95ms        ? ?/sec    1.04    106.4±2.17ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.01     48.8±0.45ms        ? ?/sec    1.00     48.2±0.82ms        ? ?/sec
bundle/bundle@threejs10x                                            1.00    433.6±4.22ms        ? ?/sec    1.00    431.9±2.91ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.00   1071.2±6.44ms        ? ?/sec    1.01  1078.3±10.81ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.00   1255.1±8.49ms        ? ?/sec    1.00  1259.0±10.91ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.00    505.1±3.55ms        ? ?/sec    1.00    506.4±5.88ms        ? ?/sec
remapping/remapping                                                 1.04     27.2±0.33ms        ? ?/sec    1.00     26.1±0.12ms        ? ?/sec
remapping/render-chunk-remapping                                    1.00     64.0±0.37ms        ? ?/sec    1.01     64.8±0.93ms        ? ?/sec
scan/scan@rome-ts                                                   1.01     96.3±1.20ms        ? ?/sec    1.00     95.7±1.17ms        ? ?/sec
scan/scan@threejs                                                   1.00     31.7±0.36ms        ? ?/sec    1.00     31.6±0.42ms        ? ?/sec
scan/scan@threejs10x                                                1.00    318.1±3.25ms        ? ?/sec    1.00    317.6±3.29ms        ? ?/sec

@IWANABETHATGUY IWANABETHATGUY force-pushed the 04-17-fix_omit_missing_export_diagnostic_when_importee_module_is_ts_tsx branch from 3b73ae3 to 6ff517f Compare April 17, 2025 05:59
@IWANABETHATGUY
Copy link
Member Author

IWANABETHATGUY commented Apr 17, 2025 •
edited
Loading

Would it be more straight to just enable shimMissingExports: true for #4144 (comment)?

I think we are trying to solve different problems,
shimMissingExports is fine for fixing that test panic, but it doesn't make sense to the user(A diagnostic is not accurate but also terminate the building process), after #4144 the MissingExport diagnostic is not accurate anymore when importee is ts/tsx, so I change severity to Warning when importee is ts/tsx and keep other as is.

@IWANABETHATGUY IWANABETHATGUY changed the title fix: omit missing_export diagnostic when importee module is ts/tsx fix: convert severity of missing_export diagnostic to warning when importee module is ts/tsx Apr 17, 2025
@IWANABETHATGUY IWANABETHATGUY added this pull request to the merge queue Apr 17, 2025
@IWANABETHATGUY
Copy link
Member Author

IWANABETHATGUY commented Apr 17, 2025

For potential false positive diagnostics when importee is ts or tsx we could give more clearer note message.

@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Apr 17, 2025
Closed
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 17, 2025
@IWANABETHATGUY IWANABETHATGUY merged commit 57ae80d into main Apr 17, 2025
29 checks passed
@IWANABETHATGUY IWANABETHATGUY deleted the 04-17-fix_omit_missing_export_diagnostic_when_importee_module_is_ts_tsx branch April 17, 2025 07:31
@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Apr 17, 2025
Merged
@sapphi-red sapphi-red mentioned this pull request Jul 29, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 29, 2025
@sapphi-red
fix: missing_export warning should not be an hard error (#5493)
04a2ddd 
#4147 changed the severity but it was still pushed to `self.errors`
instead of `self.warnings`.
@sapphi-red sapphi-red mentioned this pull request Sep 9, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request Sep 9, 2025
@sapphi-red
feat: improve MISSING_EXPORT warning to suggest type modifier (#6085
3f55bee 
)

There are multiple cases that `MISSING_EXPORT` happens even when TypeScript does not output an error.

1. #4144 (comment)
    - This case only happens when the importer and the importee is both TS. This is because we treat `foo` as a "type" rather than a "value" (since we remove `export default foo` completely. If we treat it as a "value", we should keep that).
2. The case written in https://www.typescriptlang.org/tsconfig/#verbatimModuleSyntax (`export { A } from './a'`)
    - This case only happens when the importer and the importee is both TS. This is because if `A` is a "value", `A` would exist after transpilation.

Both of these cases can be fixed by adding the `type` modifier. While setting `verbatimModuleSyntax: true` would make TypeScript to error on these cases, it changes some other behaviors, and is not straightforward to turn that on. This PR improves the error message, so that users can know what to do even if they don't enable `verbatimModuleSyntax`.

refs #4147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hyf0 hyf0 hyf0 approved these changes

Assignees

No one assigned

Labels

needs-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@IWANABETHATGUY @hyf0