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

Using local assign in Object Semigroup #42

Merged
merged 3 commits into from
May 17, 2018
Merged

Using local assign in Object Semigroup #42

merged 3 commits into from
May 17, 2018

Conversation

taras
Copy link
Member

@taras taras commented May 15, 2018

It turns out that extend-shallow does not work as expected because it actually defaults to Object.assign when available. We need to always use our own implementation of Object.assign inside of Object Semigroup to prevent React Native from overwriting our assign with theirs.

The implementation that I added is just a little slower than Object.assign.
screen shot 2018-05-15 at 1 27 15 pm

I can get it to be little faster than Object.assign, but I need to replace Object.keys(source).concat(Object.getOwnPropertySymbols(source)) with the following,

function allKeys(o) {
  let keys = Object.keys(o), 
      keysLength = keys.length,
      symbols = Object.getOwnPropertySymbols(o),
      symbolsLength = symbols.length,
      arr = Array(keysLength + symbolsLength),
      totalLength = keysLength + symbolsLength;

  for (let j = 0; j < keysLength; j++) {
    arr[j] = keys[j];
  }
  for (let j = keysLength; j < totalLength; j++) {
    arr[j] = symbols[totalLength - j - 1];
  }
  return arr;
}

I decided to not use it because it's pretty ugly, but we can always add it back.

@taras taras requested a review from cowboyd May 16, 2018 14:26
* Analogue of Object.assign(). Copies properties from one or more source objects to
* a target object. Existing keys on the target object will be overwritten.
* Modified to support copying Symbols.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a comment in here this exists because React Native does weird things?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good idea.

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. You know a library is arriving when there are disgusting code blocks there to work around platform-specific constraints. This is actually a good thing.

Since we are only using assign here, and not elsewhere, can we just make it operate on two arguments? That should tighten the loop a little bit and then it isn't more abstract than it needs to be.

@taras
Copy link
Member Author

taras commented May 16, 2018

@cowboyd done!

@taras taras merged commit 1122a5d into master May 17, 2018
@taras taras deleted the tm/use-local-assign branch May 17, 2018 10:56
@cowboyd
Copy link
Member

cowboyd commented May 17, 2018

I think we can even make the target of {} implicit. i.e. instead of assign({}, a, b) just assign(a,b)

@cowboyd
Copy link
Member

cowboyd commented May 17, 2018

lol. If this were ramda, they'd probably call it append0 or _append to indicate that it's really a "bootstrap" append.

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