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 isarray dep. #137

Merged
merged 1 commit into from
Sep 24, 2016
Merged

Remove isarray dep. #137

merged 1 commit into from
Sep 24, 2016

Conversation

jdalton
Copy link
Contributor

@jdalton jdalton commented Sep 23, 2016

Hi @feross!

The isarray package is being deprecated since IE < 11 is no longer supported by MS & IE 8 was the last browser that needed it. For the fringes, folks tend to use something like es-shim.

The Array.isArray method is supported by all Node versions, Rhino, iOS, Android, and headless browser enviros (like PhantomJS v1, SlimerJS).

With that in mind this PR removes it from buffer.

Note: I don't think test fails are related to this change.

@kgryte
Copy link

kgryte commented Sep 23, 2016

One of the selling points is that this module supports everything from IE6 and up. I see no reason to remove this dep.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 23, 2016

IE6 has less than 0.2% market share on a dead ~16yr old OS. The buffer package ends up in a lot of other deps so while it's a nice academic exercise to expand support to ancient enviros it's being shipped to plenty of folks with little to no benefit.

@kgryte
Copy link

kgryte commented Sep 23, 2016

Backward compatibility is not an academic exercise. I am one of those 0.2% who has no other choice but to use a dead ~16yr old OS. Our image acquisition software cannot be updated to a newer OS due to driver compatibility issues. Until usage is 0% and will forever remain 0%, backward compatibility matters. People at the margins are still people, and their concerns should not be dismissed so readily in the name of modernity.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 23, 2016

Update: Clarified things a bit.

@kgryte
jQuery, Prototype, React, & h5bp have already dropped IE8 support.
To help increase adoption of ES5 APIs I'm going to test the waters a bit:

The next bump of Lodash will poison (kinda scary term) the unshimmed API so that it'll throw an error informing folks to add a shim. I don't anticipate major problems as the fringes should already be using something like es-shim. However, if there's major problems I'll revert. With everyone using shims it frees up library authors to worry about other things 🙌

@feross
Copy link
Owner

feross commented Sep 24, 2016

@jdalton I understand where you're coming from -- pruning unneeded dependencies is one of my favorite pastimes. It's something that I wish more node developers took seriously, especially for packages that just run on the server-side, a much simpler and predictable environment. Reducing install time, decreasing security attack surface area, increasing site performance, and simplifying codebases are all great benefits.

So I'm definitely on your side in the fight for "dependency hygiene". But this PR drops support for IE8 a bit prematurely, IMO.

IE 6-8 together account for .77% of the US market (.08% IE6 + .07% IE7 + .62% IE8). There are 269.35 million US internet users, so this PR leaves behind 2.07 million people. And that's just in the US.

If the complexity or file size savings were meaningful, I could justify it. But this only saves 3 lines of code. Is this really the right tradeoff?

@feross
Copy link
Owner

feross commented Sep 24, 2016

@jdalton To help move this along the next version of Lodash will poison the API.

If I understand this correctly -- you're saying that going forward anyone who uses lodash on IE8 will get an intentionally broken Array.isArray? And that broken Array.isArray will also infect and break our usage of the isarray package?

Is this really necessary?

@mxstbr
Copy link

mxstbr commented Sep 24, 2016

EDIT: I was wrong, my bad! "It prevents anyone else from easily detecting that Array.isArray is missing."

@feross
Copy link
Owner

feross commented Sep 24, 2016

@mxstbr He called it "poisoning the API" for a reason. It prevents anyone else from easily detecting that Array.isArray is missing. So packages like isarray will use lodash's "helpful" implementation instead of a shim.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 24, 2016

@feross

If I understand this correctly -- you're saying that anyone who uses lodash on IE8 will get an intentionally broken Array.isArray going forward?

Naw. It's more of a show of intent since currently Lodash v4 doesn't work on an unshimmed IE8 in the first place (requires es-shim).

Lodash v3 on the only hand has legacy support out of the box. It may make more sense to implement the experiment there.

@feross
Copy link
Owner

feross commented Sep 24, 2016

It's more of a show of intent since currently Lodash v4 doesn't work on an unshimmed IE8 in the first place.

Hmm, I see. So, you're saying that lodash users already need to use es-shim anyway to make it work in IE8. I get that.

I still feel like doing a hostile takeover of the Array.isArray and breaking other people's packages (who do include a shim) isn't necessary.

If you want to help unshimmed IE8 users, you can just print them a helpful console warning or throw an exception as soon as lodash is required.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 24, 2016

@feross
It's my hope that by having a popular library take the plunge it'll help remove any hangups you may have or others may have. If folks complain then I'll revert but I don't anticipate major issues.

@feross
Copy link
Owner

feross commented Sep 24, 2016

it helps remove any hangups you may have

I think it's fine to drop support for older browsers in a major version of buffer. No hangups. That's valid semver. It's just that, as it stands, this PR is a net loss because it removes browser support and adds nothing else.

If we start using indexOf instead of handwritten for-loops to reduce code complexity and file size, and other IE9+ niceties, then we can justify dropping the older users in the next version. (Of course, people can just continue using their current version of browserify if they want older browser support). If you or someone else wants to take a stab at that, I'm happy to merge it.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 24, 2016

@feross

I think it's fine to drop support for older browsers in a major version of buffer. No hangups. That's valid semver.

Sweet! Would you dig creating a v5 branch to apply PRs to? (I'll change the base of this one)

If you or someone else wants to take a stab at that, I'm happy to merge it.

Sure thing, happy to help!

@feross
Copy link
Owner

feross commented Sep 24, 2016

@jdalton Okay, I created a v5 branch. Want to re-send this PR?

@feross feross closed this Sep 24, 2016
@jdalton
Copy link
Contributor Author

jdalton commented Sep 24, 2016

@feross Thanks!

Github introduced a feature so there's no need to close and re-send. I can just change the target of this one. If you re-open it should be pointing to the v5 branch.

@jdalton jdalton changed the base branch from master to v5 September 24, 2016 01:08
@feross feross reopened this Sep 24, 2016
@jdalton
Copy link
Contributor Author

jdalton commented Sep 24, 2016

🤘

- Super fast. Backed by Typed Arrays (`Uint8Array`/`ArrayBuffer`, not `Object`)
- Extremely small bundle size (**5.04KB minified + gzipped**, 35.5KB with comments)
- Excellent browser support (IE 6+, Chrome 4+, Firefox 3+, Safari 5.1+, Opera 11+, iOS, etc.)
- Excellent browser support (IE, Edge, Chrome, Firefox, Safari, Opera, iOS, etc.)
Copy link
Owner

@feross feross Sep 24, 2016

Choose a reason for hiding this comment

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

Thanks for adding Edge to the list here, but can you keep the browser versions for all but Edge, Chrome, and Firefox (the evergreen browsers). That is useful information.

Copy link
Contributor Author

@jdalton jdalton Sep 24, 2016

Choose a reason for hiding this comment

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

No prob!

Would you like to list the supported IE version by MS or technically by the code?
(it means either IE11 or IE9).

Do you consider Opera and Safari evergreen?

Copy link
Owner

@feross feross Sep 24, 2016

Choose a reason for hiding this comment

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

Let's list IE11. If we also drop IE9 and IE10 while we're at it, we can kill off the Object implementation of Buffer entirely, and just leave the Uint8Array implementation which would reduce file size significantly.

IE10 technically has Uint8Array but it's buggy, so it uses the Object implementation.

Opera is evergreen, but Safari is not because it doesn't have a silent autoupdater.

@feross feross merged commit 76435c5 into feross:v5 Sep 24, 2016
@jdalton jdalton deleted the isarray branch September 24, 2016 01:28
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

Successfully merging this pull request may close these issues.

4 participants