-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
Check if startsWith is configurable #702
Conversation
This will throw an exception if the property is not configurable
: that.slice(index, index + search.length) === search; | ||
} | ||
}); | ||
if(Object.getOwnPropertyDescriptor(String.prototype, "startsWith").configurable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the property is not configurable, do nothing since it is going to fail anyway with an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would it not be configurable?
It could be non-configurable only with incorrect polyfill, like a polyfill from MDN. It would be better just to remove incorrect polyfill. |
I agree 100%. Here is my scenario (IE11)
In this case the cloud service is Microsoft 365 SharePoint (using the SharePoint Framework) and "dependency x" is the botframework-webchat es5 bundle https://github.com/microsoft/BotFramework-WebChat/tree/master/samples/01.b.getting-started-es5-bundle |
Maybe it would be better to start from fixing this bug on MDN and related sources? |
I'm not a fan of adding workarounds for other polyfills bugs - that means incorrect usage of polyfills. However, it's not the first mention of this issue and if it would be fixed in the source of the problem, I'll think about adding a workaround. |
String.startsWith polyfill will throw an exception if the existing property is not configurable. Other polyfills will likely throw an exception under the same scenario. Perhaps an update like this belongs farther up the chain.
This still needs to be tested. Wanted to start the PR process to open the discussion