Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix JSON.stringify polyfill name #181

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix JSON.stringify polyfill name #181

wants to merge 1 commit into from

Conversation

mohd-akram
Copy link

Fixes #180

JLHwung
JLHwung approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@JLHwung JLHwung requested a review from zloirock September 19, 2023 02:02
nicolo-ribaudo
nicolo-ribaudo approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mohd-akram
Copy link
Author

Fixed the tests.

nicolo-ribaudo
nicolo-ribaudo reviewed Sep 19, 2023
View reviewed changes
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.ts Outdated
@@ -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"]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

zloirock
zloirock reviewed Sep 19, 2023
View reviewed changes
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.ts Outdated
@@ -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"]),
Copy link
Member

@zloirock zloirock Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

zloirock
zloirock reviewed Sep 21, 2023
View reviewed changes
...plugin-polyfill-corejs3/test/fixtures/usage-pure/built-in-static-methods-proposals/output.js
@@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be removed with those options.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's removed because it seems to default to version 3.0. I noticed this while testing the package.

Copy link
Member

@zloirock zloirock Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@mohd-akram mohd-akram Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@zloirock zloirock Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zloirock
Copy link
Member

I think that the way solving of this issue is wrong - it's better to allow passing entries for exclusion, like exclude: ['json/stringify'] or something like that.

@mohd-akram
Copy link
Author

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.

@mohd-akram
Copy link
Author

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@zloirock zloirock zloirock left review comments

@JLHwung JLHwung JLHwung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Excluding es.json.stringify does not work
4 participants
@mohd-akram @zloirock @JLHwung @nicolo-ribaudo