Make namespace @@toStringTag "Module" non-enumerable #4378
Conversation
|
You got me confused here after the discussion in #4336: Is your ultimate goal to get rid of the output.namespaceToStringTag option and enable it by default (probably just causes overhead for nearly everyone)? Or change the default (breaking change)? |
|
Though it would make sense to improve on #4336 (comment) , but this is not what you are testing (if there is anything to do #4336 (comment)) |
oh, dang, you're right. I'll fix the test. should have revisited the issue. 😄 |
dnalborczyk
force-pushed
the
test/4336-to-string-tag
branch
from
5fb6e00
to
e38cfca
Compare
| const { configurable, enumerable, value, writable } = Object.getOwnPropertyDescriptor( | ||
| ns, | ||
| Symbol.toStringTag | ||
| ); |
There was a problem hiding this comment.
Why not explicitly check the Object.assign behaviour? Then this would also document why it is important and people do not need to Google it again.
There was a problem hiding this comment.
good idea, I'll add that to the test.
dnalborczyk
changed the title
|
also changed the issue title. |
yes, personally I would, here's to the why:
at the bare minimum I would make it |
|
I have added a fix and also additional tests so that this also applies to tags added to entry points. I also decided to use this PR to deprecate namespaceToStringTag in favour of a new option |
lukastaegert
force-pushed
the
test/4336-to-string-tag
branch
from
2717f8f
to
e2c395b
Compare
lukastaegert
changed the title
Codecov Report
@@ Coverage Diff @@
## master #4378 +/- ##
=======================================
Coverage 98.75% 98.75%
=======================================
Files 204 204
Lines 7299 7306 +7
Branches 2078 2079 +1
=======================================
+ Hits 7208 7215 +7
Misses 33 33
Partials 58 58
Continue to review full report at Codecov.
|
|
Can we support "treeshaking" for this tag itself? Eg if the namespace object is not globally exposed, to skip adding the symbol? Or do we not have comprehensive global exposure analysis to rely on here? |
|
Ah of course, looks great, and the overhead of the symbol is negligible (especially when compared to former interop wrappers!). |
…'` to CJS files (#6043) As of version 2.69.0, setting `output.generatedCode` to `'es2015'` (as we do) causes Rollup to [add `[Symbol.toStringTag]: 'Module'` to generated CJS files](rollup/rollup#4378 (comment)). Though this is valid ES6, it causes Jest to be unable to mock our generated packages. Though [a PR](jestjs/jest#13513) has been opened to fix this, the change almost certainly won't be backported, so anyone using Jest 29.2.2 or under will run into [this problem](#5994) if they try to mock `@sentry/xxx` 7.16. (The relevant change was introduced in #5951, when we (semi-)accidentally upgraded Rollup from 2.67.1 to 2.78.0, and was first included in version 7.16.) This PR prevents the new Rollup behavior, since it has no known benefit. (The [Rollup docs](https://rollupjs.org/guide/en/#outputgeneratedcode) say that presence of the `'Module'` toStringTag "is used for feature detection in certain libraries and frameworks," but not having it hasn't seemed to hurt us so far.)
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Resolves #4336
Description
Updated: This will make sure that the
Symbol.toStringTagproperty of namespaces is non-enumerable, which has an effect on the behaviour ofObject.assignand the object spread operator. This includesMoreover, this deprecates the
output.namespaceToStringTagoption in favour ofoutput.generatedCode.symbols, which again is now automatically enabled fores2015output.The deprecation warning is hidden for now but will be shown after the next major Rollup version.