safe hasOwnProperty call

[LongTengDao]

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

There will be a bug when export names include hasOwnProperty.

I'm not sure how to add test for this case...

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
rollup	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 2, 2023 2:57am

[lukastaegert]

Looks reasonable, but it is not entirely clear to me what scenario we are guarding against. Most importantly, we would need a test (probably a function test that runs the generated code) that is broken without that change.
exports is an object provided by the runtime. Why wouldn't we want to use its hasOwnProperty method?

[LongTengDao]

@lukastaegert

https://rollupjs.org/repl/?version=3.26.0&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQSUyMiUyMiUyQyUyMm1vZHVsZXMlMjIlM0ElNUIlN0IlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwKiUyMGZyb20lMjAnZXh0ZXJuYWwnJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCUyQyU3QiUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBjb25zdCUyMGhhc093blByb3BlcnR5JTIwJTNEJTIwKCklMjAlM0QlM0UlMjBmYWxzZSUzQiU1Q24lNUNuZXhwb3J0JTIwbGV0JTIweCUyMCUzRCUyMDElM0IlNUNuJTVDbnNldFRpbWVvdXQoKCklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIweCUyMCUzRCUyMDIlM0IlNUNuJTdEJTJDJTIwMCklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlMkMlMjJuYW1lJTIyJTNBJTIyZXh0ZXJuYWwlMjAoY2FzZSUzQSUyMG5vbi1nZXR0ZXIpJTIyJTdEJTJDJTdCJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMGNvbnN0JTIwaGFzT3duUHJvcGVydHklMjAlM0QlMjBmYWxzZSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSUyQyUyMm5hbWUlMjIlM0ElMjJleHRlcm5hbCUyMChjYXNlJTNBJTIwdGhyb3cpJTIyJTdEJTJDJTdCJTIyY29kZSUyMiUzQSUyMiUyRiUyRiUyMGV4dGVybmFsJTVDbmV4cG9ydCUyMGNvbnN0JTIwaGFzT3duUHJvcGVydHklMjAlM0QlMjAoKSUyMCUzRCUzRSUyMCU3QiU1Q24lMjAlMjAlMkYqJTIwZG8lMjBzb21ldGhpbmclMjBoZXJlJTIwKiUyRiU1Q24lN0QlM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlMkMlMjJuYW1lJTIyJTNBJTIyZXh0ZXJuYWwlMjAoY2FzZSUzQSUyMHNpZGUtZWZmZWN0cyklMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyY2pzJTIyJTdEJTdEJTdE

// entry.js

export * from 'external';

You will got nothing dynamic re-export:

// external (case: non-getter)

export const hasOwnProperty = () => false;

or got an error thrown:

// external (case: throw)

export const hasOwnProperty = false;

even worse with side effects:

// external (case: side-effects)

export const hasOwnProperty = () => {
  /* do something here */
};

see the generated code, and imagine what will happen in runtime:

var external = require('external');

Object.keys(external).forEach(function (k) {
	if (k !== 'default' && !exports.hasOwnProperty(k)) Object.defineProperty(exports, k, {
		enumerable: true,
		get: function () { return external[k]; }
	});
});

[lukastaegert]

Thanks for clarifying. Then you should add this as a "function" test case, then we do not need to imagine.

[codecov]

Codecov Report

Merging #5010 (fd6b228) into master (1b62c33) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5010   +/-   ##
=======================================
  Coverage   98.96%   98.96%           
=======================================
  Files         226      226           
  Lines        8348     8348           
  Branches     2290     2290           
=======================================
  Hits         8262     8262           
  Misses         31       31           
  Partials       55       55           

Impacted Files	Coverage Δ
src/finalisers/shared/getExportBlock.ts	100.00% <100.00%> (ø)

[LongTengDao]

@lukastaegert here ready if I add the test right

[lukastaegert]

Looks perfect, thanks so much for following up on this!

[rollup-bot]

This PR has been released as part of rollup@3.26.1. You can test it via npm install rollup.