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.

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

don't change string prototype #7

Closed
dylang opened this issue Jun 9, 2014 · 3 comments
Closed

don't change string prototype #7

dylang opened this issue Jun 9, 2014 · 3 comments

Comments

@dylang
Copy link
Contributor

dylang commented Jun 9, 2014

if (typeof String.prototype.startsWith !== "function") {
  String.prototype.startsWith = function (str) {
    return this.slice(0, str.length) === str;
  };
}

This means you are changing how string works for all code that depends on your code.

It would be prefered to use a function or something like underscore.string. https://github.com/epeli/underscore.string

I'm not sure if this is related, but a co-worker is seeing this:

depcheck/index.js:95
      if (usedDependency === definedDependency || usedDependency.startsWith(de
                                                                 ^
TypeError: Object 11 has no method 'startsWith'
    at node_modules/npm-check/node_modules/depcheck/index.js:95:66
@rumpl
Copy link
Member

rumpl commented Jun 9, 2014

The error your co-worker is having is odd...

What do you think about this polyfill ?

@dylang
Copy link
Contributor Author

dylang commented Jun 9, 2014

That's a good find, but I'm still not a fan of changing the prototype.

If you look at his code, the first line is

if (!String.prototype.startsWith) {

This means that if any other node module changes the prototype by adding a startsWith before yours runs, your code will be using that other code instead of that polyfill.

It just feels too risky to me to rely on.

@rumpl
Copy link
Member

rumpl commented Jun 9, 2014

Fair enough.

Will change it to a normal function.

@dylang dylang mentioned this issue Jun 11, 2014
@rumpl rumpl closed this as completed Jun 14, 2014
dylang pushed a commit to dylang/depcheck that referenced this issue Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants