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

Add isSupported property #14

Closed
wants to merge 1 commit into from
Closed

Add isSupported property #14

wants to merge 1 commit into from

Conversation

LinusU
Copy link
Collaborator

@LinusU LinusU commented Feb 3, 2018

Fixes #11

@LinusU
Copy link
Collaborator Author

LinusU commented Feb 3, 2018

On the other hand, maybe this isn't needed since as far as I know, it will always return true for our supported browsers 🤔 In that case it might not make too much sense to have this since we already have said that other browsers are too old anyways...

@tremby
Copy link

tremby commented Feb 4, 2018

But who says my site's supported browsers are the same as clipboard-copy's supported browsers?!

@LinusU
Copy link
Collaborator Author

LinusU commented Feb 5, 2018

But who says my site's supported browsers are the same as clipboard-copy's supported browsers?!

Noone is saying that, but I'm saying that it might be outside the scope of this module to provide support for those browsers.

Merging this PR would add code that we need to maintain, simply to support some very old browsers. It will also add an implication that you should check the isSupported property before using the library, which I don't think is necessary when you are targeting modern browsers.

I'm not saying it's one way or the other, but it's something that should be considered before merging this PR.

According to caniuse.com, the usage of non-supported browsers are really low. Not sure if it's useful to support that 0.81%...

Browser Usage
IE8 0.21 %
IE9 0.12 %
IE10 0.13%
Safari 9 0.24%
Chrome 29 0.11%
Total 0.81%

@tremby
Copy link

tremby commented Feb 5, 2018

Noone is saying that, but I'm saying that it might be outside the scope of this module to provide support for those browsers.

You keep talking about supporting these older browsers. Nobody is trying to do that, for the purposes of copying text. This change does not provide support for old browsers, as you must know. This change allows code to know whether or not the current browser supports copying. That's a totally different thing. You seriously think adding a test for your users to know whether or not this module is actually going to do what it's supposed to is out of scope?

Merging this PR would add code that we need to maintain

It's a few very simple lines.

It will also add an implication that you should check the isSupported property before using the library, which I don't think is necessary when you are targeting modern browsers.

That's up to the developer, isn't it? And I don't see how it's an issue. If they know they're only supporting browsers that your library supports, there's obviously no need to perform the test, though I don't see that there's any harm in doing so. If they are required to support IE10 and Safari 9 (and I note that you didn't mention any mobile browsers at all -- there are a lot of old mobile browsers still in use) then the test is very useful.

According to caniuse.com, the usage of non-supported browsers are really low. Not sure if it's useful to support that 0.81%...

Cite your source more specifically. When I look this up I get a very different story. https://caniuse.com/#feat=clipboard shows me that "partial support" is available to just 85.9% of users. You seem to be under the impression that the five browsers you listed are the only ones in use which don't support the clipboard. That is not accurate.

Besides, the authors of this library have already made this decision; they've decided (or were forced) not to support those older browsers for the purposes of copying text. That's fine, no argument there. But the authors of this library are not responsible for making the browser support decision for the website the developer is working on! It's not always even up to us. Sometimes the target market is some corporate or university or governmental environment where a large proportion of users are stuck on some old browser.

Providing this test for use cases like that makes things convenient for the developer; it means they don't need to read your source code to see the method of implementation and then look up how to test whether that's available on the current browser. It also provides peace of mind that the result of the test will remain accurate even when updating this clipboard-copy library.

@LinusU
Copy link
Collaborator Author

LinusU commented Feb 6, 2018

[...] shows me that "partial support" is available to just 85.9% of users.

good catch, I must have missed that number 🤔

(totally off topic, but what is UC browser for Android? and who is using it??)

Providing this test for use cases like that makes things convenient for the developer; it means they don't need to read your source code to see the method of implementation and then look up how to test whether that's available on the current browser. It also provides peace of mind that the result of the test will remain accurate even when updating this clipboard-copy library.

This is a good argument 👍

Again, I'm not necessarily against this change, after all, I took the time to make it 😄. I'm just a bit on the fence, but generally positive to it.

I'll give some time for @feross to chime in with what he thinks, otherwise, I'll merge this 👌

@feross
Copy link
Owner

feross commented Feb 7, 2018

@tremby The point of this library is to be more minimalist than existing solutions. I don't have any desire to add any feature requests or additional API surface area. I don't want the module size to increase at all.

Your options are:

  1. Pretend older browsers don't exist, fail ungracefully in them.

  2. Use the return value to detect whether the copy worked or not.

  3. Use another package like https://www.npmjs.com/package/clipboard which has a Clipboard.isSupported() method to detect in advance if copying is supported.

  4. Fork this and do whatever you want.

@feross feross closed this Feb 7, 2018
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.

3 participants