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 modify string prototype #159

Closed
patrickarlt opened this issue Sep 8, 2014 · 18 comments
Closed

Don't modify string prototype #159

patrickarlt opened this issue Sep 8, 2014 · 18 comments
Assignees
Milestone

Comments

@patrickarlt
Copy link

Ran into an issue debugging an issue with a coworker.

If the string module is included anywhere downstream of shelljs you get a pair of warnings.

to: wrong arguments

toEnd: wrong arguments

Here is the script to duplicate the issue.

var shelljs = require('shelljs');
var S = require('string');

// will produce output...

This has already been caught by several other issues...

Since shelljs is so heavily relied upon it might be a better to not modify the global String prototype.

@kimmobrunfeldt
Copy link

Having the same issue. Please fix

@colinbes
Copy link

Same issue, any solutions or proposed work arounds?

@davelnewton
Copy link

+1, driven by the string.js report.

@jprichardson
Copy link

Yeah, this is super annoying and violates modern idiomatic JavaScript :) Any willingness to fix this?

@jprichardson
Copy link

A bit quick on the keyboard... hope that didn't come off as entitled; if it did, I'm sorry.

Even an option to disable it would work great. Thanks!

@cookch10
Copy link

cookch10 commented Mar 5, 2015

Also experiencing this issue. Please fix.

@steveoh
Copy link

steveoh commented Mar 26, 2015

to and toEnd prototype violations. This should be fairly simple to fix?

@jprichardson
Copy link

I've published a new version 3.1.1 of string.js that should work around this problem.

@myndzi
Copy link

myndzi commented Jun 2, 2015

Extending the prototype at all is dubious, especially implicitly instead of explicitly, but doing so with functions that dirty the filesystem is just nasty. I just spent two hours of my day running down where a mysterious file was coming from that's a result of what appears to be nasty behavior on the part of both shelljs AND string.js; either fix will do, but they both need it.

@nfriedly
Copy link

nfriedly commented Aug 5, 2015

+1 This looks to be the root cause of watson-developer-cloud/node-sdk#56 / jprichardson/string.js#160

@az7arul
Copy link

az7arul commented Aug 5, 2015

version 3.3.1 of string.js now ignores the to and toEnd methods to get around this problem

@garaboncias
Copy link

+1 pleasse do not modify or at least make it not enumerable.
Object.defineProperty(String.prototype, 'to', {enumerable: false});
Object.defineProperty(String.prototype, 'toEnd', {enumerable: false});

@nfischer nfischer self-assigned this Jan 28, 2016
@nfischer nfischer added this to the v1.0.0 milestone Jan 28, 2016
@nfischer
Copy link
Member

Bumping this thread, since this will finally be resolved in #360. Thanks for reporting this!

@nfischer
Copy link
Member

Even an option to disable it would work great. Thanks!

We decided to go with the "opt-out" approach for ShellJS v0.7 (see #398 for the discussion). If you don't mind having the string prototype extended, you can continue to use the convenience script require('shelljs/global'). If this is a concern for you, you can use var shell = require('shelljs') instead, which neither modifies the prototype nor pollutes the global namespace.

If the string prototype is still a concern for shelljs/global, please comment on this thread and we can consider removing that for ShellJS v0.8.

@BYK
Copy link
Contributor

BYK commented Mar 20, 2016

@nfischer - I think modifying builtins or their prototypes is a very bad idea. I'd encourage deprecating the whole global thing for the next version TBH. (and yeah string prototype modification too)

@ariporad
Copy link
Contributor

@BYK: I think the idea behind global mode is to more closely emulate bash scripting in shelljs. If you're writing a simple script which is only shelljs commands, it can save a lot of time. This is also why we have the non-global option for people who would prefer not to.

@BYK
Copy link
Contributor

BYK commented Mar 21, 2016

@ariporad - we can still do that with require thought right? And with ES6 imports (I know, not ready yet) it is even easier.

Anyways, I think even if we keep the global mode, having our own String class and modifying its prototype is way safer than touching a built-in's prototype.

@nfischer
Copy link
Member

The rationale is that modifying the global namespace is equally as wrong as modifying the builtin. So, for now, it's been added back, but only for global. I think we should give a deprecation warning (converting a string to a ShellString is an alternative). This is something I wouldn't be opposed to removing for good in v0.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests