Fix JSON.stringify polyfill name #181
Conversation
mohd-akram
force-pushed
the
fix-json-stringify
branch
from
6de8ed4
to
61cd3f0
Compare
|
Fixed the tests. |
| @@ -385,7 +385,7 @@ export const StaticProperties: ObjectMap2<CoreJSPolyfillDescriptor> = { | |||
| "es.object.create", | |||
| "es.object.freeze", | |||
| ]), | |||
| stringify: define("json/stringify", ["es.json.stringify"], "es.symbol"), | |||
| stringify: define("json/stringify", ["es.json.stringify"]), | |||
There was a problem hiding this comment.
It's possible that the reason there was es.symbol is that when polyfilling JSON.stringify we also have to polyfill symbols. In that case, this definition should be
| stringify: define("json/stringify", ["es.json.stringify"]), | |
| stringify: define("json/stringify", ["es.json.stringify", "es.symbol"]), |
Let's wait for @zloirock to see this PR before merging :)
| @@ -385,7 +385,7 @@ export const StaticProperties: ObjectMap2<CoreJSPolyfillDescriptor> = { | |||
| "es.object.create", | |||
| "es.object.freeze", | |||
| ]), | |||
| stringify: define("json/stringify", ["es.json.stringify"], "es.symbol"), | |||
| stringify: define("json/stringify", ["es.json.stringify"]), | |||
There was a problem hiding this comment.
It's not a bug or something like that. In early versions of core-js@3, JSON.stringify logic was only in es.symbol module to fix JSON.stringify with symbols. After that, many other fixes of JSON.stringify logic, well-formed unicode, JSON.rawJSON support, etc. were added and the main part of JSON.stringify polyfill was moved to es.json.stringify.
For modern core-js versions, es.symbol here is not required, however, it could break compatibility with early core-js@3 versions.
mohd-akram
force-pushed
the
fix-json-stringify
branch
from
61cd3f0
to
db02e94
Compare
| @@ -2,7 +2,6 @@ var _Array$from = require("core-js-pure/features/array/from.js"); | |||
| var _Array$isArray = require("core-js-pure/features/array/is-array.js"); | |||
| var _Array$of = require("core-js-pure/features/array/of.js"); | |||
| var _Date$now = require("core-js-pure/features/date/now.js"); | |||
| var _JSON$stringify = require("core-js-pure/features/json/stringify.js"); | |||
There was a problem hiding this comment.
It should not be removed with those options.
There was a problem hiding this comment.
I think it's removed because it seems to default to version 3.0. I noticed this while testing the package.
There was a problem hiding this comment.
This entry is available in 3.0.
IIRC it could be removed because of the order of modules in the array of dependencies - here the first is es.json.stringify that wasn't in 3.0 - however, it does not mean that core-js-pure/features/json/stringify is not required.
There was a problem hiding this comment.
It checks getModulesByVersion (something like that, which references a JSON file with that metadata) in core-js-compat and es.json.stringify is not mentioned under 3.0. I think that's why.
For pure I believe it only checks the first entry in the array since a pure import will fetch its own dependencies.
There was a problem hiding this comment.
Yes, because JSON.stringify polyfill was only in es.symbol and it was exported in core-js-pure/features/json/stringify. Which is what we're talking about above.
|
I think that the way solving of this issue is wrong - it's better to allow passing entries for exclusion, like |
|
This PR is to fix the exclusion :D I think perhaps the library should default to using the latest version of core-js rather than 3.0. |
|
Any update on this? |
Fixes #180