Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix create-property.js #1324

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Conversation

stonechoe
Copy link
Contributor

This PR is for #1322

The changes are as follows

module.exports = function (object, key, value) {
  var propertyKey = toPropertyKey(key);
  if (propertyKey in object) definePropertyModule.f(object, propertyKey, createPropertyDescriptor(0, value));
  else object[propertyKey] = value;
};
module.exports = function (object, key, value) {
  var propertyKey = toPropertyKey(key);
  definePropertyModule.f(object, propertyKey, createPropertyDescriptor(0, value));
};

I am not certain about the impact of this patch on performance - in case the existing code was intentionally written for performance benefits like JIT optimization, please handle it appropriately at your discretion.

@zloirock
Copy link
Owner

zloirock commented Jan 25, 2024

This PR should be to master branch.

You are right, it's a performance optimization. Without this optimization, some methods worked unacceptably slow. However, now most methods where it's used are available in most engines, and Proxy is enough popular. I will think make it sense to accept it or not.

@stonechoe stonechoe reopened this Jan 26, 2024
@stonechoe stonechoe changed the base branch from v4 to master January 26, 2024 04:27
@stonechoe
Copy link
Contributor Author

Sorry for the confusion. I changed the base branch to master.

@zloirock zloirock merged commit 034b7cd into zloirock:master Jan 31, 2024
28 checks passed
@stonechoe
Copy link
Contributor Author

Thank you for your hard work for maintaining this project. May I ask a question?

Does the "unacceptable slowness" come from the fact that Object.defineProperty(object, key, createPropertyDescriptor(0, value)) cannot be optimized by engine, whereas object[key] = value can?
Or does it only occur when polyfill version is used for definePropertyModule.f?

@zloirock
Copy link
Owner

zloirock commented Feb 5, 2024

Does the "unacceptable slowness" come from the fact that Object.defineProperty(object, key, createPropertyDescriptor(0, value)) cannot be optimized by engine, whereas object[key] = value can?

Yes. Some years ago it was dozens of times slower even in modern engines. I remember issues about optimizing this case in bug trackers of some engines. I didn't check the performance recently, so I don't know how it works in actual versions.

@stonechoe stonechoe deleted the fix/issue-createproperty branch February 22, 2024 04:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants