Skip to content
This repository was archived by the owner on Sep 10, 2020. It is now read-only.

Update lodash.bind to version 4.0.0 🚀 #11

Merged
merged 5 commits into from
Jan 14, 2016

Conversation

greenkeeperio-bot
Copy link
Contributor

Hello 👋

🚀🚀🚀

lodash.bind just published its new version 4.0.0, which is not covered by your current version range.

If this pull request passes your tests you can publish your software with the latest version of lodash.bind – otherwise use this branch to work on adaptions and fixes.

Happy fixing and merging 🌴


This pull request was created by greenkeeper.io.
It keeps your software, up to date, all the time.

Tired of seeing this sponsor message? Upgrade to the supporter plan! You'll also get your pull requests faster ⚡

@wraithgar
Copy link
Contributor

This is being used only in one place, which is in the browser handler of lib/connection.js. Updating this would, I think, mean losing IE 6-8 support.

At a glance, this build appears to be breaking because of a timing issue where a dependency of lodash-bind wasn't on npm yet.

@wraithgar
Copy link
Contributor

Kicked off the builds again, will wait to merge this to give those IE6 fans time to speak up.

@pdf
Copy link
Contributor

pdf commented Jan 13, 2016

If you're going to lose IE6-8 support anyway, you may as well just drop the dep and use ES5 bind

@wraithgar
Copy link
Contributor

You're right, and when I went to look at the rest of lib/connection I remembered why it's like this. The entire reason we're using this (notice how we use es5 bind in non-browser) is because [email protected] (our test suite) does not support this ariya/phantomjs#10522

@wraithgar
Copy link
Contributor

Additionally, because of build problems on os x and linux, it won't be available on npm untill 2.1 Medium/phantomjs#288

@pdf
Copy link
Contributor

pdf commented Jan 13, 2016

A different approach might be to just pull in a polyfill - should solve the problem, and the polyfill is unlikely to change or be deprecated for the foreseeable future.

@wraithgar
Copy link
Contributor

Yeah I'm pretty torn on this. After my work on amp I'm pretty gun shy about re-implementing things like this. I think ultimately the problem of removing this is separate from the smaller one of "should we update"

I think we can just update, since there's no real expectation of support below IE9 here anyways, and wait for [email protected] (Or some other solution we haven't thought of) before removing it altogether.

@wraithgar
Copy link
Contributor

Instead of using lodash.bind in the main code, just override function.prototype.bind
during the browser test code
Switch to eslint-config-andyet/es5
@wraithgar
Copy link
Contributor

So, since we're only concerned about the fact that phantomjs doesn't have the polyfill, not whether or not the user browser does in production, we moved the shim into the test script. I think this works.

Really not concerned about legacy browser support.

It was too easy to confuse with the exported browser sub library
@wraithgar
Copy link
Contributor

I renamed the browser test file so as to not be confused with the exported browser sub library in this module.

@pdf
Copy link
Contributor

pdf commented Jan 13, 2016

Oh right, I was indeed confused!

Remove apparently stale mention of assert and explain file's
purpose a litle better
@wraithgar
Copy link
Contributor

thanks for being a sounding board for this @pdf I'm pretty happy with where it ended up. I'll merge this tomorrow morning (PST)

wraithgar added a commit that referenced this pull request Jan 14, 2016
Update lodash.bind to version 4.0.0 🚀
@wraithgar wraithgar merged commit 6435520 into master Jan 14, 2016
@wraithgar wraithgar deleted the greenkeeper-lodash.bind-4.0.0 branch January 14, 2016 21:47
@wraithgar
Copy link
Contributor

Websockets weren't available pre IE10 so this isn't a breaking change, patch release

http://caniuse.com/#search=websocket

@wraithgar
Copy link
Contributor

published as v2.0.2

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

Successfully merging this pull request may close these issues.

3 participants