Namespace object of reexported external star export

[guybedford]

Currently external star exports are not supported when reexported through namespace objects -

https://rollupjs.org/repl?version=0.58.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG0lMjBmcm9tJTIwJy4lMkZtYXRocy5qcyclM0IlNUNuJTVDbmNvbnNvbGUubG9nKG0pJTNCJTIyJTdEJTJDJTdCJTIybmFtZSUyMiUzQSUyMm1hdGhzLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmV4cG9ydCUyMColMjBmcm9tJTIwJ2V4dGVybmFsJyUzQiUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJjanMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

I'd suggest we should output the following here instead:

'use strict';

var external = require('external');

var m = /*#__PURE__*/Object.freeze(Object.assign({}, external));

console.log(m);

effectively chaining Object.assign calls per reexported star exported module, in the precedence order, possibly with a final step to ignore the default export.

[lukastaegert]

Sounds good 👍

[guybedford]

The better output here to match semantics is of course:

import * as externalNs from 'external';

const ns = {
  export: 'name'
};
for (let p in externalNs)
  if (!(p in ns) && p !== 'default') ns[p] = externalNs[p];

Note that the for in is necessary as ES module namespaces don't provide hasOwnProperty(x) === true!

[lukastaegert]

The better output here to match semantics is of course

Could you elaborate a little more in what way the previous approach is not spec compliant. In any case, the second approach would not require an Object.assign polyfill in old environments. We should still Object.freeze, though.

[guybedford]

Object.assign, like Object.keys wont work on module namespace objects. In addition, the precedence conditional for not overwriting local exports or the default export is needed.

[lukastaegert]

Thx

[guybedford]

Just tested and object.keys does work, but I think browsers aren't implementing this correctly at the moment, I believe (although I may be wrong) these are supposed to fail as namespaces don't support [[OwnPropertyKeys]], but it's being discussed at the spec level - tc39/ecma262#1296.

[lukastaegert]

See also #2489

[Westbrook]

🤞

[shellscape]

@guybedford @lukastaegert thoughts on this one? Object.keys should be good to go in browsers since the OP date.

[guybedford]

This is very much a still-valid spec compliance bug.

[lukastaegert]

Agree. Just a note reading the above code, to my knowledge in case there is more than one namespace reexport, conflicting names should throw a specific error when accessed. Not sure if we would want to replicate this, though.

[lukastaegert]

At least at the moment we throw an error, which is better than outputting nonsense.

[guybedford]

Ah I missed that we now have an error here. This could be closed then I suppose.

[lukastaegert]

Or we fix it 😉

[shellscape]

I'll add this to the middle-aged bug board

[guybedford]

This was resolved in PR #3474.