feat(object): object.entries polyfill

[zewa666]

adds a polyfill for IE > 9

closes issue #63

[zewa666]

@EisenbergEffect here we are, sorry for the large modification, it seems there were no linting rules applied before so the auto-fix performed some additional work.

[bigopon]

@zewa666 @EisenbergEffect I remember we refused to add this polyfill once or twice in the past, because it's not what needed to operate Aurelia. This will cost everyone who doesn't use aurelia-store

[zewa666]

It wouldn't be a problem adding this polyfill to the store plugin itself. I'm open to any way

[EisenbergEffect]

Probably better to either just add this one to the store plugin directly or to specific in the docs that you should "bring your own". Also, any way to not use this API? That might be the simplest, depending on the details of your implementation.

[zewa666]

Okidoki, I guess the BYOP (bring-your-own-polyfill) approach, lol, is definitely good enough and something that could be easily documented.

[3cp]

@EisenbergEffect if you changed mind, let's reopen this one.
aurelia/documentation#409

[EisenbergEffect]

Ok, I've been back and forth on this...so I apologize. I think we'll just put this in :)

[EisenbergEffect]

@bigopon Do we need to make some fixes to our CI for this repo? This PR has some build errors. We haven't changed polyfills in a while, so maybe it's out of date? If you know offhand, let me know.

[3cp]

@fkleuver could you have a look of this circleci error?

[bigopon]

Please do not add this polyfill. We are punishing non-store users for no reason. I think best we polyfill it in store itself

[3cp]

Come on, only 10 lines.

[3cp]

There are plenty of aurelia code use Object.keys.

For example this one

const keys = Object.keys(current);
        for (let j = 0, jj = keys.length; j < jj; ++j) {
          const value = current[keys[j]];
          // note: we could remove this if-branch and call this.register directly
          // - the extra check is just a perf tweak to create fewer unnecessary arrays by the spread operator
          if (isRegistry(value)) {
            value.register(this);
          } else {
            this.register(value);
          }
        }

can be reduced to

Object.entries(current).forEach(([key, value]) => {
  // note: we could remove this if-branch and call this.register directly
  // - the extra check is just a perf tweak to create fewer unnecessary arrays by the spread operator
  if (isRegistry(value)) {
    value.register(this);
  } else {
    this.register(value);
  }
});

You probably can save more than 10 lines after refactoring all aurelia code using this new polyfills.

[bigopon]

:👍

[fkleuver]

Regarding circleci, the source branch is 31 commits behind master so circleci cannot run the branch that this PR is based on. Should update that branch

[fkleuver]

@3cp

Object.entries(current).forEach(([key, value]) => {
  // note: we could remove this if-branch and call this.register directly
  // - the extra check is just a perf tweak to create fewer unnecessary arrays by the spread operator
  if (isRegistry(value)) {
    value.register(this);
  } else {
    this.register(value);
  }
});

I would definitely not do that on any code that is (or may potentially be) a hot path because it creates tons of additional objects. One new array for each value..

Frankly I would much rather see that we simply refrained from using Object.entries instead of polyfilling it, but that's just my 2c.

I guess aurelia-store doesn't have this problem as much because application data flow tends to be less active than for example binding, but putting it in other places in the core will likely significantly increase memory consumption and generally slow things down. So, why not just keep it to aurelia-store?

[zewa666]

This is creating far too much discussion for such a minimal change. How about we first go with adding the polyfill to the store plugin itself and later on, if needed, we still can extract it to polyfills?

[EisenbergEffect]

@zewa666 That sounds reasonable. Let's go with that for now.