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

fix(angular.copy): should be able to angular.copy Float32Array #10745

Closed
wants to merge 1 commit into from
Closed

fix(angular.copy): should be able to angular.copy Float32Array #10745

wants to merge 1 commit into from

Conversation

DropsOfSerenity
Copy link
Contributor

angular.copy can now copy a Float32Array when only provided src. I added some tests to confirm this.

fixes #10210

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@DropsOfSerenity
Copy link
Contributor Author

@googlebot done.

@googlebot
Copy link

CLAs look good, thanks!

@@ -0,0 +1,7 @@
@ngdoc error
@name ng:cpfad
@fullName Copying Float32Array
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Float32Array/%TypedArray%/g

@petebacondarwin
Copy link
Contributor

+1 @caitp

I was just looking at this too and this is just what I was thinking.
If we are going to support copying Float32Array then we should support all type arrays. Unfortunately IE9 doesn't support them so we need to tread carefully but @caitp's ideas are the direction to go.

@DropsOfSerenity - if you would like to update your PR accordingly that would be great.

  • Update the error doc to be more generic
  • Create a isTypedArray() method instead of specific one. Perhaps we could compare src.constuctor.name with /(Uint8(Clamped)?)|(Uint16)|(Uint32)|(Int8)|(Int16)|(Int32)|(Float(32)|(64))Array/
  • Update the copy method accordingly
  • Update the docs

@DropsOfSerenity
Copy link
Contributor Author

Excellent, I'll get to it, on a side-note I don't know why the CI build keeps failing, all the tests pass over here.

@petebacondarwin
Copy link
Contributor

Don't worry. We are having flakiness issues connecting to SauceLabs and BrowserStack when running tests on Travis

@DropsOfSerenity
Copy link
Contributor Author

@caitp @petebacondarwin

Alright thanks for the great advice, I've added a new commit that supports all TypedArrays. I did try using constructor.name instead however that resulted in a memory leak for reasons I don't understand.

One other approach I was thinking of could be to use instanceOf, but then I'd have to add a check to make sure the Type being compared to actually exists (in the case of ie9- it doesn't)

Either way this solution seems to be working well, updated the tests to check all TypedArrays.

@DropsOfSerenity
Copy link
Contributor Author

@caitp sweet looks like CI finally passed :)

@caitp
Copy link
Contributor

caitp commented Jan 29, 2015

party =) I'll take a look at this later today, gotta finish up a quick patch I'm writing first

@@ -0,0 +1,7 @@
@ngdoc error
@name ng:cpfad
Copy link
Contributor

Choose a reason for hiding this comment

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

should be @name ng:cpta

@caitp
Copy link
Contributor

caitp commented Jan 29, 2015

LGTM

I lied, I guess I did review the PR before finishing my patch :3 It looks good to me, just fix up the nits and squash the commits

@caitp caitp self-assigned this Jan 29, 2015
* angular.copy can now copy a FixedArrays when only provided src.
@DropsOfSerenity
Copy link
Contributor Author

@caitp Ok i squashed with my changes. :3

@caitp
Copy link
Contributor

caitp commented Jan 29, 2015

good stuff, looks ready to merge to me

@caitp
Copy link
Contributor

caitp commented Jan 29, 2015

i guess this was really a feature rather than a fix >_- oh well, not changing that message :>

@caitp caitp closed this in aa0f644 Jan 29, 2015
@Hypercubed
Copy link
Contributor

This change is great... have you considered exposeding istypedArray as angular.isTypedArray?

@caitp
Copy link
Contributor

caitp commented Feb 28, 2015

glad you like it @Hypercubed! but... exposing these utilities is problematic in many ways =\ i don't think it would be smart... Besides, @@toStringTag makes it unreliable anyhow

petebacondarwin pushed a commit that referenced this pull request Nov 19, 2015
angular.copy can now copy a %TypedArray%s.

Limitations: It is not possible to update the length of a %TypedArray%, so currently an error is thrown
if the destination object is a %TypedArray%. However, it is possible to change values in a typed array,
so in the future this may only be a problem if the length of the source and destination is different.

Closes #10745
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

angular.copy doesn't work with typed arrays
5 participants