Remove Ramda dependency in S.trim #201
Conversation
| @@ -3104,7 +3104,7 @@ | |||
| def('trim', | |||
| {}, | |||
| [$.String, $.String], | |||
| R.trim); | |||
| function(s) { return s.trim(); }); | |||
There was a problem hiding this comment.
I think String.prototype.trim is a relatively new addition to JavaScript. IE8 doesn't support it, is that a concern?
There was a problem hiding this comment.
I guess that's David's call. I look at the Ramda implementation and it seems insane to go to those lengths to support a browser like IE8.
There was a problem hiding this comment.
Personally I'm for writing ES5 and documenting that fact; a user has the option of a shim if desired for the use case
There was a problem hiding this comment.
My position on this matter may seem quirky, but I think it's justified.
- I would like to remove Ramda as a dependency.†
- I'm not opposed to dropping support for ES3.
- I wouldn't support keeping the Ramda dependency solely to preserve ES3 compatibility.
- We're not yet in a position to remove the Ramda dependency, so I see no reason to break ES3 compatibility here.
So, when we remove the Ramda dependency I'll be absolutely fine with making this change. Until then, though, I see no harm in taking advantage of the "insane" measures R.trim takes to support old browsers. :)
If we decide to add S.trimLeft and S.trimRight at some point, I'll reassess my position.
† I even have a branch locally on which I attempted to remove Ramda. It's certainly possible, but a few things need to happen first. For example, sanctuary-def needs to expose its show function to save us from duplicating the definition of R.toString a second time.
There was a problem hiding this comment.
My position is that we ought to rip the plaster off as soon as possible. This change is a breaking change, the longer we leave it the more disruptive it is for our users when we do pull the plug. I believe we should be front leading these sorts of updates.
There was a problem hiding this comment.
[T]he longer we leave [this change] the more disruptive it is for our users when we do pull the plug.
It seems the other way around to me. In a year, say, the number of people whose browsers don't provide String#trim will be lower than it is today, presumably.
I don't feel strongly about this change, though, so if others are in favour I don't mind merging it.
There was a problem hiding this comment.
Actually that's a very good point. I hadn't thought of it like that. Maybe we should hold off then.
There was a problem hiding this comment.
In a year, say, the number of people whose browsers don't provide String#trim will be lower than it is today, presumably.
That's probably true, but I think the number of organisations / apps that want to support ES3-only browsers changes much less smoothly and more unpredictably
|
I'm going to close this pull request. I'd love to see a pull request which removes the Ramda dependency entirely, but I don't see the value in making this change in isolation. If at any point you're interested in taking on the task of removing the Ramda dependency, let me know so I can share a patch which may save you a couple of hours. :) |
|
I'm generally not a fan of big bang PRs like that, I reckon we'll just let the work of #145 continue and the dependency will gradually be excised. |
|
May I resurrect this PR? |
|
Yes, please do. :) |
|
Rebased against master... |
|
🌳 |
As a member of the council for de-Ramdification, I hearby declare this function purged.