Skip to content

Conversation

maartenbreddels
Copy link
Contributor

supersedes #2693 fixes #2692 , feedback welcome,

@jashkenas
Copy link
Owner

Looking good! Let's add one more check to make sure ArrayBuffer is an object that exists in the runtime before starting, and I think we're there.

@jashkenas jashkenas added the bug label Aug 18, 2017
@maartenbreddels
Copy link
Contributor Author

Yes I see the error. What's the best way to check. window.ArrayBuffer or root.ArrayBuffer? And I also have to skip the tests, there I don't have a reference to root, can I use window?

@jashkenas
Copy link
Owner

Why don't you test and cache a typeof ArrayBuffer check up at the top of Underscore, and then just check the variable within the deepEquals. You can't use window.

@jashkenas
Copy link
Owner

Great. Do you mind cleaning up the formatting and making it nice -- like the rest of the surrounding code? Spaces near parens, semicolons, and so on...

@maartenbreddels
Copy link
Contributor Author

Yeah, installing eslint as we speak, the only issue that I don't know to tackle yet is node 0.8 which doesn't seem to support isView.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.801% when pulling 81e5bf7 on maartenbreddels:typed_array_eq into 20e7c6e on jashkenas:master.

@jashkenas
Copy link
Owner

the only issue that I don't know to tackle yet is node 0.8 which doesn't seem to support isView.

It doesn't? That seems like a bit of a dealbreaker.

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage increased (+0.05%) to 96.812% when pulling d73f20c on maartenbreddels:typed_array_eq into 20e7c6e on jashkenas:master.

@akre54
Copy link
Collaborator

akre54 commented Sep 7, 2017

Do we care to support node 0.8? Last release was 2014.

@jashkenas
Copy link
Owner

Do we care to support node 0.8? Last release was 2014.

Fair enough, then.

@jasongrout
Copy link

Do we care to support node 0.8? Last release was 2014.

Fair enough, then.

Moot point since d73f20c solves that case too, right?

@vidartf
Copy link

vidartf commented May 16, 2018

It seems like everyone was OK with this PR. Any chance of getting it merged?

@jgonggrijp
Copy link
Collaborator

@jashkenas Could you have another look and confirm that you still approve of this? If you do, I can take care of the merge conflicts.

I can also add isTypedArray to the exports and polish its implementation a little bit, if you like. Personally, I'd make that a condition for merging.

@jgonggrijp jgonggrijp requested a review from jashkenas April 30, 2020 23:25
@jashkenas
Copy link
Owner

Sounds like a fine plan to me. It would also be great if we could drop that big list of toString fallback name checks, if they’re not necessary for modern JS environments anymore.

@jashkenas jashkenas removed their request for review May 1, 2020 03:38
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request May 2, 2020
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request May 2, 2020
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request May 2, 2020
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request May 2, 2020
@jgonggrijp jgonggrijp merged commit fa0429b into jashkenas:master May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isEqual incorrectly compares DataView objects
7 participants