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

Provide better error when creating a namespace object containing a reexported external namespace #2499

Merged
merged 3 commits into from Oct 9, 2018

Conversation

lukastaegert
Copy link
Member

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:

Description

Currently, it is not possible to create an explicit namespace object for a module that reexports the namespace of an external module. Doing this properly would definitely require a new logic for creating namespaces as we would now need to combine an existing namespace with new exports.
Until a solution is found, this PR will throw a proper error instead of either creating garbage
https://rollupjs.org/repl?version=0.64.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG4lMjBmcm9tJTIwJy4lMkZtb2R1bGVfMS5qcyclM0IlNUNuY29uc29sZS5sb2cobiklM0IlMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIybW9kdWxlXzEuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwKiUyMGZyb20lMjAnZXh0ZXJuYWwnJTNCJTVDbmV4cG9ydCUyMGNvbnN0JTIweCUyMCUzRCUyMDElM0IlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXNtJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

or failing with a type error
https://rollupjs.org/repl?version=0.66.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG4lMjBmcm9tJTIwJy4lMkZtb2R1bGVfMS5qcyclM0IlNUNuY29uc29sZS5sb2cobiklM0IlMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIybW9kdWxlXzEuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwKiUyMGZyb20lMjAnZXh0ZXJuYWwnJTNCJTVDbmV4cG9ydCUyMGNvbnN0JTIweCUyMCUzRCUyMDElM0IlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXNtJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great, note the issue tracking the fix is in #2165.

@lukastaegert
Copy link
Member Author

Thanks for the link, I was wondering about this but did not search properly.

@lukastaegert lukastaegert merged commit 0707a10 into master Oct 9, 2018
@lukastaegert lukastaegert deleted the reexport-external-error branch October 9, 2018 05:24
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.

None yet

3 participants