bug: @@toStringTag property undefined, should be 'Module'

[dnalborczyk]

Rollup Version

2.62.0

Operating System (or Browser)

n/a

Node Version (if applicable)

n/a

Link To Reproduction

https://rollupjs.org/repl/?version=2.62.0&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG5zJTIwZnJvbSUyMCcuJTJGZm9vLmpzJyUzQiU1Q24lNUNuY29uc29sZS5sb2cobnMlNUJTeW1ib2wudG9TdHJpbmdUYWclNUQlMjAlM0QlM0QlM0QlMjAnTW9kdWxlJyklMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJmb28uanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwY29uc3QlMjBmb28lMjAlM0QlMjAnYmFyJyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

// index.js
import * as ns from './foo.js';

console.log(ns[Symbol.toStringTag] === 'Module')

// foo.js
export const foo = 'bar'

rollup bundle:

const foo = 'bar';

var ns = /*#__PURE__*/Object.freeze({
	__proto__: null,
	foo: foo
});

console.log(ns[Symbol.toStringTag] === 'Module');

Expected Behaviour

prints: true

Actual Behaviour

prints: false

[kzc]

esbuld is no different in this regard:

$ esbuild index.js --bundle | node
false

How would you model the bundle in order for such a thing to work? By explicitly setting that property in the generated namespace?

$ cat 0.js
const foo = 'bar';

var ns = /*#__PURE__*/Object.freeze({
	__proto__: null,
	[Symbol.toStringTag]: "Module",
	foo: foo
});

console.log(ns[Symbol.toStringTag] === 'Module');

$ node 0.js
true

Does any code in the wild rely on this construct?

[dnalborczyk]

esbuld is no different in this regard:

$ esbuild index.js --bundle | node
false

it's a bug in esbuild as well 😉

How would you model the bundle in order for such a thing to work? By explicitly setting that property in the generated namespace?

$ cat 0.js
const foo = 'bar';

var ns = /*#__PURE__*/Object.freeze({
	__proto__: null,
	[Symbol.toStringTag]: "Module",
	foo: foo
});

that might as well be all what's needed, yeah.

Does any code in the wild rely on this construct?

hard to know. it's in the spec, I would expect that a bundler should (ideally) not alter the runtime. it's a potential easy fix and aligns rollup with the spec and other bundlers: https://sokra.github.io/interop-test/#import--as-x (edit: could be that the sokra interop tests are only comparing differences with esm <-> cjs, not esm <-> esm)

[dnalborczyk]

out of curiosity, I was wondering how rollup would fare running against test262. I gave it a try and ran with a subset (only specific feature flags, e.g. module, top-level-await etc.) of the tests as rollup doesn't distinguish between script and module code (as far as I know). Generally speaking the result looks quite amazing overall.

[kzc]

I don't know about test262, but a while back rollup was run against uglify-js' ES6+ test suite and the dozen or so failures were filed and fixed. The script tests were also skipped, as rollup cannot handle them. We also dealt with most of the typical TDZ issues in rollup several months ago.

I'd personally suggest to hold off adding [Symbol.toStringTag]: "Module" to the namespace until a user reports the issue in real world code. And even then, to only inject it if the property or string literal toStringTag appears in any input.

Are there any other special ES module Symbol import properties?

[dnalborczyk]

I don't know about test262

it's the TC39 ecmascript language spec test suite. that's the test suite language implementers run against.

but a while back rollup was run against uglify-js' ES6+ test suite and the dozen or so failures were filed and fixed.

interestingly I added the rollup-terser plugin on top in a subsequent run which unfortunately resulted in more failures. I'll try to file those issues in terser eventually as well. not sure how uglify would be doing, I gotta check [eventually as well].

I'd personally suggest to hold off adding [Symbol.toStringTag]: "Module" to the namespace until a user reports the issue in real world code.

I don't know, is that a good strategy? I would think one would preemptively fix any known bugs. I don't think it would be great if say chrome or node.js would follow a similar strategy: "wait until it's been reported".

And even then, to only inject it if the property or string literal toStringTag appears in any input.

you mean if anyone is actually reading the property? sure, if that's possible. I don't know how much overhead that would add? 🤷‍♂️

Are there any other special ES module Symbol import properties?

I'm fairly sure that's it (I might have to report back on that one).

the test is here btw: https://github.com/tc39/test262/blob/main/test/language/module-code/namespace/Symbol.toStringTag.js

[kzc]

it's the TC39 ecmascript language spec test suite. that's the test suite language implementers run against.

Thanks, I was familiar with the test suite but not its results with Rollup.

I don't know, is that a good strategy? I would think one would preemptively fix any known bugs. I don't think it would be great if say chrome or node.js would follow a similar strategy: "wait until it's been reported".

There are outright bugs, and then there are features that are not supported. NodeJS and Chrome have to be 100% ES spec compliant down to the most obscure feature. Rollup has a number of known limitations like the aforementioned lack of non-strict mode handling. I think this issue falls in that category.

you mean if anyone is actually reading the property? sure, if that's possible. I don't know how much overhead that would add?

In my opinion the small overhead to scan all AST string literals and properties would be worth it not to include the 30 odd bytes [Symbol.toStringTag]: "Module" to every imported namespace for this rarely used feature.

[lukastaegert]

The output.namespaceToStringTag option is there to address this. It is decidedly an opt-in for various reasons.

[kzc]

Good stuff.

Next time I'll grep before I jump into a discussion.
;-)

[dnalborczyk]

The output.namespaceToStringTag option is there to address this. It is decidedly an opt-in for various reasons.

thank you @lukastaegert I had no clue, way too many options 😄 I'll give it a try ...

edit: seems to work 🚀 , although, a small tiny nit: the property would need to be non-enumerable.

since the code-gen is using Object.create(null) for the namespace object (non-namespace-string-tag) already, it should be fairly simple to make that prop non-enumerable as well.

Object.create(null, {
	[Symbol.toStringTag]: { enumerable: false, value: 'Module' }
});

I'll have a look into a PR if that's ok.

[lukastaegert]

Acutally considering your last remark, Symbol properties are always non-enumerable, so there should be nothing to do (unless you can write a test that proves otherwise)

Update: I did my googling and you are of course right, there is a difference in behaviour for Object.assign({}, namespace) as well as {...namespace} as enumerable Symbols are indeed copied in those cases https://stackoverflow.com/a/31030427/11576601

[dnalborczyk]

there is a difference in behavior for Object.assign({}, namespace) as well as {...namespace} as enumerable Symbols are indeed copied in those cases stackoverflow.com/a/31030427/11576601

interesting, I did not know that.

[dnalborczyk]

just for posterity:

const { freeze, getOwnPropertyDescriptor } = Object

console.log(
	getOwnPropertyDescriptor({ [Symbol.toStringTag]: 'Module' }, Symbol.toStringTag)
)

// prints:
{
  value: 'Module',
  writable: true,
  enumerable: true,
  configurable: true
}

console.log(
	getOwnPropertyDescriptor(freeze({ [Symbol.toStringTag]: 'Module' }), Symbol.toStringTag)
)

// prints:
{
  value: 'Module',
  writable: false,
  enumerable: true,
  configurable: false
}