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

Remove Ramda dependency in S.trim #201

Merged
merged 1 commit into from
May 19, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
Original file line number Diff line number Diff line change
@@ -3302,7 +3302,7 @@
def('trim',
{},
[$.String, $.String],
R.trim);
function(s) { return s.trim(); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think String.prototype.trim is a relatively new addition to JavaScript. IE8 doesn't support it, is that a concern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm for writing ES5 and documenting that fact; a user has the option of a shim if desired for the use case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's a very good point. I hadn't thought of it like that. Maybe we should hold off then.

Copy link
Contributor

@rjmk rjmk Apr 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


//# words :: String -> Array String
//.