Skip to content

feat: throw friendly warning message for ambiguous export#1634

Merged
hyf0 merged 15 commits into
rolldown:mainfrom
shulaoda:feat/friendly-ambiguous-export
Jul 16, 2024
Merged

feat: throw friendly warning message for ambiguous export#1634
hyf0 merged 15 commits into
rolldown:mainfrom
shulaoda:feat/friendly-ambiguous-export

Conversation

@shulaoda

@shulaoda shulaoda commented Jul 15, 2024 •
edited by hyf0
Loading

Copy link
Copy Markdown
Member

Closes #1621.
Closes #397.

@netlify

netlify Bot commented Jul 15, 2024 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 8b0fb8c
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66963a5fd2a8900008cbeb49

@IWANABETHATGUY IWANABETHATGUY marked this pull request as draft July 16, 2024 03:02
@IWANABETHATGUY

IWANABETHATGUY commented Jul 16, 2024

Copy link
Copy Markdown
Member

Can you fix all compilation errors before we can push forward?

IWANABETHATGUY
IWANABETHATGUY reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown_error/src/events/ambiguous_external_namespace.rs Outdated
IWANABETHATGUY
IWANABETHATGUY reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown/tests/fixtures/errors/ambiguous_external_namespace/artifacts.snap Outdated
IWANABETHATGUY
IWANABETHATGUY reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown_error/src/events/ambiguous_external_namespace.rs Outdated
@hyf0 hyf0 linked an issue Jul 16, 2024 that may be closed by this pull request
Throw friendly warning message for ambiguous export #1621
Closed
hyf0
hyf0 reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown_error/src/events/ambiguous_external_namespace.rs Outdated
@shulaoda shulaoda marked this pull request as ready for review July 16, 2024 06:01
@shulaoda shulaoda requested a review from IWANABETHATGUY July 16, 2024 06:17
hyf0
hyf0 reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown/tests/fixtures/errors/ambiguous_external_namespace/_config.json Outdated
Comment thread crates/rolldown/src/stages/link_stage/bind_imports_and_exports.rs Outdated
@shulaoda shulaoda requested a review from hyf0 July 16, 2024 07:38
@hyf0

hyf0 commented Jul 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This is fine to be merged. Could you run just test-rust and update all changed snapshots by cargo insta accept.

@IWANABETHATGUY

IWANABETHATGUY commented Jul 16, 2024

Copy link
Copy Markdown
Member 
diff --git a/crates/rolldown/tests/esbuild/dce/disable_tree_shaking/_config.json b/crates/rolldown/tests/esbuild/dce/disable_tree_shaking/_config.json
index cc39d9bb..94d0fe2d 100644
--- a/crates/rolldown/tests/esbuild/dce/disable_tree_shaking/_config.json
+++ b/crates/rolldown/tests/esbuild/dce/disable_tree_shaking/_config.json
@@ -8,7 +8,7 @@
     ],
     "treeshake": false,
     "external": [
-      "react"
+      "react/jsx-runtime"
     ]
   },
   "expectExecuted": false

to avoid changes of crates/rolldown/tests/esbuild/dce/disable_tree_shaking/artifacts.snap

@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jul 16, 2024
Merged
@IWANABETHATGUY IWANABETHATGUY enabled auto-merge July 16, 2024 08:49
hyf0
hyf0 reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown/tests/fixtures/warnings/ambiguous_external_namespace/_config.json Outdated
hyf0
hyf0 reviewed Jul 16, 2024
View reviewed changes
Comment thread crates/rolldown/tests/fixtures/warnings/ambiguous_external_namespace/artifacts.snap
---
# warnings

## AMBIGUOUS_EXTERNAL_NAMESPACES

@hyf0 hyf0 Jul 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://hyrious.me/esbuild-repl/?version=0.23.0&b=e%00entry.js%00import+%7B+foo+%7D+from+%27.%2Fproxy%27%0Aconsole.log%28foo%29&b=%00a.js%00export+const+foo+%3D+1&b=%00b.js%00export+const+foo+%3D+2&b=%00proxy.js%00export+*+from+%27.%2Fa%27%0Aexport+*+from+%27.%2Fb%27&o=--bundle+--splitting+--format%3Desm+--external%3Aext+--outdir%3Ddist

Esbuild treats this as an error and has better report. Let's merge this PR for now. I'll open an new issue to port esbuild's behavior.

@shulaoda @hyf0
Update crates/rolldown/tests/fixtures/warnings/ambiguous_external_nam…
4da65b1 
…espace/_config.json

Co-authored-by: Yunfei He <i.heyunfei@gmail.com>
auto-merge was automatically disabled July 16, 2024 08:57

Head branch was pushed to by a user without write access

@hyf0 hyf0 enabled auto-merge July 16, 2024 09:16
hyf0
hyf0 approved these changes Jul 16, 2024
View reviewed changes

@hyf0 hyf0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

@hyf0 hyf0 mentioned this pull request Jul 16, 2024
Closed
@hyf0 hyf0 added this pull request to the merge queue Jul 16, 2024
Merged via the queue into rolldown:main with commit d03e15b Jul 16, 2024
@shulaoda shulaoda deleted the feat/friendly-ambiguous-export branch July 16, 2024 09:35
github-merge-queue Bot pushed a commit that referenced this pull request Jul 16, 2024
@IWANABETHATGUY
refactor: rename BuildError to BuildDiagnostic (#1641)
77b107b 
<!-- Thank you for contributing! -->

### Description
1. follow #1634, since we have
https://github.com/rolldown/rolldown/pull/1634/files#diff-f68450ab0a8cfe082912fd82beb770be4085872ef174fd398796eb4eff2e8aceR237.
<!-- Please insert your description here and provide especially info
about the "what" this PR is solving -->
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

@IWANABETHATGUY IWANABETHATGUY Awaiting requested review from IWANABETHATGUY

Assignees

No one assigned

Labels

needs-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Throw friendly warning message for ambiguous export

3 participants

@shulaoda @IWANABETHATGUY @hyf0