Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(copy): add support for String/Boolean/Number object types #13641

Closed
wants to merge 1 commit into from
Closed

fix(copy): add support for String/Boolean/Number object types #13641

wants to merge 1 commit into from

Conversation

jbedard
Copy link
Collaborator

@jbedard jbedard commented Dec 28, 2015

By modifying isString/isBoolean/isNumber this also effects everywhere those are used, which I think is good? But could also effect performance in those areas if called an extreme number of times.

Fixes the original issue mentioned in #13398

@lgalfaso
Copy link
Contributor

I am not sure about this PR change to isXXX can we refactor this so
toString is called once on the object?

On Monday, December 28, 2015, Jason Bedard [email protected] wrote:

By modifying isString/isBoolean/isNumber this also effects everywhere
those are used, which I think is good? But could also effect performance in
those areas if called an extreme number of times.

Fixes the original issue mentioned in #13398

#13398

You can view, comment on, or merge this pull request online at:

#13641
Commit Summary

  • fix(copy): add support for String/Boolean/Number object types

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#13641.

@jbedard jbedard force-pushed the 13398-types branch 2 times, most recently from e296d7c to 76623d0 Compare December 29, 2015 03:43
@jbedard
Copy link
Collaborator Author

jbedard commented Dec 29, 2015

I've updated the PR to only make the toString call once.

Original commit for reference: 5dc679d

lgalfaso
lgalfaso reviewed Dec 29, 2015
View reviewed changes
src/Angular.js
} else if (isFunction(source.cloneNode)) {
destination = source.cloneNode(true);
} else {
} else if (!(destination = copyType(source))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will not work as intended with new Boolean(false) nor with new String('')

@jbedard
Copy link
Collaborator Author

jbedard commented Dec 30, 2015

Updated again

@lgalfaso
Copy link
Contributor

LGTM

@lgalfaso lgalfaso mentioned this pull request Dec 30, 2015
Closed
@lgalfaso lgalfaso closed this in 7b51243 Dec 30, 2015
@zhukov zhukov mentioned this pull request Jun 29, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jbedard @lgalfaso @googlebot