search

sindresorhus / is-observable

Check if a value is an Observable
MIT License
39 stars 7 forks source link

Updated function to be compliant of other environments #9

Closed fahad19 closed 6 years ago

fahad19 commented 6 years ago

What's done

Updated the index.js file to avoid using arrow functions.

If asked, I can rewrite it in a single line like before.

Why?

This helps when using the library in non-nodejs environments.

Maintainer's opinion

I understand this package aims to be targeting NodeJS only, and has opinions about not supporting ES5 as mentioned in #8.

You created the package, and you have every right to stick to your decisions.

Given it is a very popular library, I would kindly request you to consider accepting this PR for the ease of adoption by the wider community who want to use it in the browsers too (including old browsers).

NodeJS users are not affected in any negative way by doing this change. ~In fact, it is now exporting a named function.~

With this PR, we are enabling users targeting other environments to use this library too without requiring any additional configuration tweaks in their bundler (like Webpack).

Thanks for the great work!

SamVerschueren commented 6 years ago

I'm :-1: on this. As explained in #8, this is not something we want to do. There are Webpack plugins that can solve this for you. Also, create-react-app supports this in their latest version (maybe still beta though).

fahad19 commented 6 years ago

You are totally right. There are ways to handle this in webpack. Without even any plugins actually, just by using “module.exclude”, which involves developer to write rules in regex.

It’s all possible to overcome bundling issue. We are not blocked for this PR.

I’m just trying to understand what we gain by not making this change? Is it the number of lines in this package? Size? Performance?

sindresorhus commented 6 years ago

I’m just trying to understand what we gain by not making this change? Is it the number of lines in this package? Size? Performance?

Maintainer happiness. I don't care about browser use. It creates so much churn. For example, what browsers do I support then? You left const in there. It's not supported by IE. I'm sure I'll get requests to change that too. Browsers require bundling regardless, as you can't send lots of individual packages down the network.