DamonOehlman / filestream

W3C File Reader API streaming interfaces
30 stars 11 forks source link

fix: restore IE 11 support #21

Closed petermikitsh closed 5 years ago

petermikitsh commented 5 years ago

Added babel to ship a build that supports IE 11.

Source files are now kept in the src directory. To prevent these changes from breaking users who import via require('filestream/read') or require('filestream/write'), src files are transpiled to their original location-- the project's root directory.

I've verified the correct files are included when publishing by runnning npm pack --dry:

npm notice 
npm notice 📦  filestream@5.0.0
npm notice === Tarball Contents === 
npm notice 1.2kB package.json           
npm notice 226B  .airtap.yml            
npm notice 188B  .babelrc               
npm notice 59B   docs.json              
npm notice 568B  index.js               
npm notice 3.3kB read.js                
npm notice 2.6kB README.md              
npm notice 3.2kB write.js               
npm notice 939B  examples/drag-n-drop.js
npm notice 816B  test/read.js           
npm notice === Tarball Details === 
npm notice name:          filestream                              
npm notice version:       5.0.0                                   
npm notice filename:      filestream-5.0.0.tgz                    
npm notice package size:  4.3 kB                                  
npm notice unpacked size: 13.0 kB                                 
npm notice shasum:        0bd336a2fbcce67e08b478bbe4bfce606aed93d9
npm notice integrity:     sha512-1wj8G2yL8VkVr[...]YQsnHetEk7Ztg==
npm notice total files:   10                                      
npm notice 
filestream-5.0.0.tgz
feross commented 5 years ago

Thanks for the PR, but I would rather not support IE11 in v5 of this library. The extra complexity of a build step is just not worth it, IMO. IE11 has 1.81% marketshare and it's declining rapidly.

If you must still support IE11, then you can simply depend on v4 of this library which still works perfectly.

petermikitsh commented 5 years ago

The build step automatically runs when you go to publish. There is no overhead in the management of this repository.

petermikitsh commented 5 years ago

Also, 1.8% marketshare might not sound like much, but it's critical to support if your application has a large user base and important customers. I would consider reverting #20 as it reduced the number environments this package works in, without adding additional value.

feross commented 5 years ago

There is no overhead in the management of this repository.

Ehh, adding additional dependencies and build tooling is not free. There is some overhead to maintaining that. I'm sorry that we dropped support for a browser that you still need to support, but maintaining that support indefinitely is not free.

Also, 1.8% marketshare might not sound like much, but it's critical to support if your application has a large user base and important customers.

If you have a large user base and very important customers, then it sounds like you have the resources necessary to maintain a fork of this package for your company's use. Instead, you're asking volunteers to support legacy browsers on behalf of your for-profit company :/

Why not simply continue relying on v4 which works perfectly, and which you were more than happy to use just a few days ago?

I would consider reverting #20 as it reduced the number environments this package works in, without adding additional value.

20 removed two dependencies which are no longer needed in modern JS environments. That makes this package easier to maintain, which is a win to me.

I'll defer to @DamonOehlman here, since he's the OG maintainer. But I don't think asking users to rely on v4 for IE11 support is unreasonable.

petermikitsh commented 5 years ago

I didn't ask volunteers to support legacy browsers, as I opened this PR myself. That's why I wanted to contribute this minor, non-breaking change to the community. @feross, I know you maintain a number of successful projects, so I'm sure you may have some empathy for the IE11 requirement, or work on projects that have that requirement. Having for-profit companies contribute, use and value the module certainly won't hurt the longevity and overall maintenance of a project, as you'd then have a larger base of maintainers to rely on.

If any changes do occur on master, I hope the 4.x maintains parity with 5.x.

The two modules removed in #20 have an incredibly small footprint. xtend is 233 bytes. inherits is 262 bytes. So, it's a very minor value-add. You can see, given my requirements, how I could view it as a step back.

Thank you again for considering my PR. I will continue using the 4.x branch.

feross commented 5 years ago

@petermikitsh Thank you for your gracious reply. I appreciate your understanding. I'm happy to help publish improvements to the v4.x branch, especially if you're willing to help backport the changes. :) I don't expect too many changes.