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

Throw proper error when indirectly reexporting a recursive binding #4472

Merged
merged 3 commits into from Apr 30, 2022

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Apr 23, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
resolves #4459

Description

Rollup is throwing a stack overflow when a binding is imported in a file that is exported in that file under the same name, see https://rollupjs.org/repl/?version=2.70.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QmZvbyU3RCUyMGZyb20lMjAlNUMlMjIuJTJGbWFpbi5qcyU1QyUyMiUzQiU1Q24lNUNuZXhwb3J0JTIwJTdCZm9vJTdEJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

This PR extends the logic that was already in place to detect the case for direct reexports
https://rollupjs.org/repl/?version=2.70.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMCU3QmZvbyU3RCUyMGZyb20lMjAnLiUyRm1haW4uanMnJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

to also cover indirect ones (i.e. import and then export of the same binding).

@github-actions
Copy link

@github-actions github-actions bot commented Apr 23, 2022

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#gh-4459-circular-reexport

or load it into the REPL:
https://rollupjs.org/repl/?pr=4472

@codecov
Copy link

@codecov codecov bot commented Apr 23, 2022

Codecov Report

Merging #4472 (c4fe9d8) into master (e3bfe69) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4472   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files         205      205           
  Lines        7327     7331    +4     
  Branches     2080     2082    +2     
=======================================
+ Hits         7236     7240    +4     
  Misses         33       33           
  Partials       58       58           
Impacted Files Coverage Δ
src/Module.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3bfe69...c4fe9d8. Read the comment docs.

@lukastaegert lukastaegert enabled auto-merge (squash) Apr 30, 2022
@lukastaegert lukastaegert disabled auto-merge Apr 30, 2022
@lukastaegert lukastaegert merged commit 2bbdc8e into master Apr 30, 2022
12 checks passed
@lukastaegert lukastaegert deleted the gh-4459-circular-reexport branch Apr 30, 2022
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.

1 participant