theKashey / proxyequal

There is a bit more smart way to compare things, than a shallow equal.
MIT License
72 stars 7 forks source link

ProxyPolyfill is unconditionally imported #15

Open markerikson opened 5 years ago

markerikson commented 5 years ago

The current implementation always imports ProxyPolyfill:

https://github.com/theKashey/proxyequal/blob/f4692072301e63a7287e7cfbb0ac0cbc0feccfef/src/index.js#L4-L8

However, that polyfill is 6K before minification, which looks like it's about 1/3 of the total source size.

Per https://bundlephobia.com/result?p=proxyequal@2.0.3 , the deployed size is 8.3K min and 3K min+gz. If the polyfill was only conditionally imported (or even better, made a separate thing that users could import and enable themselves if needed), that size could probably shrink noticeably.

theKashey commented 5 years ago

Yeah, that's the problem - how to make common (IE11 compatible) bundle work, and also be able to use more optimized solution. Unfortunately Proxy could not polyfilled, thus is not a part of core-js/babel-polyfill, so I could not defer this decision to a customer.

Once someone will find a proper solution for shipping ES6 code within the library - I'll do it. It's not the hard task to wrap polyfill with some static condition, dead-code-eliminate it later, but there is no such condition to use yet :(

theKashey commented 5 years ago

Double checked - proxy polyfill is 800b after gzip, which is 1/3 of all size.

markerikson commented 5 years ago

Given that this is a lib entirely dedicated to proxy usage, perhaps it is worth leaving that up to the end user? If they really need to target an ES5 environment, they could either add the official Google Proxy polyfill themselves, or do something like:

import {proxyEqual} from 'proxyequal';
import {polyfillProxy} from 'proxyequal/polyfill'

polyfillProxy()
luisherranz commented 5 years ago

+1 for that.

Having separate bundles for es5 and es6 is something that is gaining a lot of adoption lately: https://philipwalton.com/articles/deploying-es2015-code-in-production-today/

theKashey commented 5 years ago

This is just dangerous. Nobody will test applications in IE11 after adding a new library, thus breaking our old mate. We did it 3 times last year after migration to Cypress.

I would feel better if I would be able to remove unnecessary code, not to ask (who is reading readme of non direct dependencies) user to import (when? How?) a polyfill.

luisherranz commented 5 years ago

Sure, polyfills are usually managed automatically by @babel/polyfill so users don't have to worry about them, but this is an uncommon case because this polyfill is not feature complete and therefore not part of babel. As far as I know, there is not an standard way to deal with this type of polyfills yet.

Another option would be to export the es6 version in another entry point:

import { proxyEqual } from 'proxyequal';
import { proxyEqual } from 'proxyequal/es6'; // doesn't contain the es5 polyfill

Default version still contains the polyfill and if you know what you are doing, you can opt-in to the non-polyfilled version.

theKashey commented 5 years ago

The problem is with desision location. This is not a library you gonna ever use - this library would be used by another library, and even that library could be not the "last". How then ask the final customer to pick the right version? What if by some circumstances you will get a two version of this library?

So - the only "real" way to tackle this - add a new type of not quite real polyfills to the babel. I'll fill the issue, let's see what they would say.

luisherranz commented 5 years ago

Support in babel-polyfill would be ideal of course. If you open the issue let us know to show our support.

But if that doesn't happen, or while it happens, I think dependent libraries can choose for themselves between:

import memoize from 'memoize-state/es6';
import { useReduxState } from 'reactive-react-redux/es6';

For example, we want to use proxyequal (or memoize-state, not sure yet) in a project which won't have ie11 support (because the way we use Proxy for mutations) and therefore we will be including the polyfill unnecessarily.

When SEO requirements are high, each Kb saved helps you with your Lighthouse metrics. Even if people want ie11 support they will probably end up doing the <script type="module"> trick to send es6 javascript to modern browsers and in that case having a separate /es6 entry point will be quite useful because you can use the normal import in your code:

import memoize from 'memoize-state';

and an alias in the Webpack of es6 files:

resolve: {
  alias: {
    "memoize-state": "memoize-state/es6"
  }
}
theKashey commented 5 years ago

There is no way to inform a final customer about things which shall be done.

We have a few ways:

// feature list does not exists, but it could look like this if(!process.features || !process.features.proxy) { code of polyfill };

- babel polyfill could or import a _global_ polyfill, or, again, cut a piece of text.
```js
 new Proxy() => + import 'proxy-polyfill';

It would be not a big deal to create a webpack Plugin, something like the Environment one with features for the current target. It's really piece a cake, thanks to can-i-use. But webpack is not the single bundler we are using.

Babel could be not the best option, as long as it usually is not applied to node_modules. But again - it's not a big deal to create a plugin for it.

Other options are probably not valuable.

I wondering - may be @ai have got a few ideas.

luisherranz commented 5 years ago

Another trick I've used in the past to get rid of promise libraries inside other packages when the environment contains a native Promise is to use an alias to a package called promise-monofill, which is simply:

module.exports = Promise;

https://github.com/rsp/node-promise-monofill/blob/master/index.js

In this case, I guess it could work with something like:

  resolve: {
    alias: {
      "proxyequal/src/proxy-polyfill.js": "proxy-monofill"
    }
  }

There is no way to inform a final customer about things which shall be done.

I don't quite agree with that because people should RTFM, but I agree with: