Allow to safely probe external namespaces

[lukastaegert]

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 #3996

Description

It is actually not safe to replace namespace imports with named imports in the case of external dependencies as the behaviour will be different if a named export does not exist: While for the namespace import, a property will just be undefined, the missing named import will throw at parse time.

This changes the behaviour to always keep external namespaces as objects and keep the property access. This allows to safely probe for the existence of named imports.

[rollup-bot]

Thank you for your contribution! ❤️

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

npm install rollup/rollup#gh-3996_external_namespace_probing

or load it into the REPL:
https://rollupjs.org/repl/?circleci=14553

[codecov]

Codecov Report

Merging #3999 (9bccc4f) into master (7fbda1a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3999      +/-   ##
==========================================
+ Coverage   97.22%   97.24%   +0.01%     
==========================================
  Files         191      191              
  Lines        6745     6741       -4     
  Branches     1970     1967       -3     
==========================================
- Hits         6558     6555       -3     
  Misses         99       99              
+ Partials       88       87       -1     

Impacted Files	Coverage Δ
src/ast/nodes/MemberExpression.ts	99.15% <100.00%> (+0.78%)	⬆️

---

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 7fbda1a...9bccc4f. Read the comment docs.

[stefcameron]

Thanks! Your repro helped me a lot to quickly spot what was missing. As we are now keeping external namespace accesses as property accesses, the logic to detect if accessing a property has a side-effect was kicking in, keeping the expressions in the code. However as namespace objects are not normal objects, it is not possible that a property of a namespace is a side-effectful getter. Fix at #4001.

My pleasure, glad I could help you, and the community! 🎉 Thank you for digging into it.