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

Typed Arrays not truncated #441

Closed
jwagner opened this issue May 8, 2015 · 6 comments
Closed

Typed Arrays not truncated #441

jwagner opened this issue May 8, 2015 · 6 comments

Comments

@jwagner
Copy link

jwagner commented May 8, 2015

Chai does currently not handle typed arrays very well:
http://jsfiddle.net/7LbaveL6/

Typed Arrays have various uses but are usually big blobs of data (for example from canvas getImageData).

The problem with the way they are currently handled is that it causes the page to freeze when assertions are run on objects containing bigger typed arrays.

I looked at the code but wasn't sure where the right place for a fix would be.

In my opinion it would be ok to just output the to String representation of the typed array (by treating it like a primitive), but it could also use the array code path with proper truncation.

@keithamus
Copy link
Member

Thanks for the issue @jwagner, I also completely agree.

Right now, all code for object inspection lives in lib/chai/utils/inspect.js (I say "right now", because there are plans to finally migrate it over into https://github.com/chaijs/loupe).

To add support for Typed Arrays should be reasonably easy to fix. If I were to fix this, I'd simply create a function called isTypedArray(value) which returns true if it is an instance of one of the Typed Array constructors. Then just above the isArray call (L126), run the check using pretty much the same if statement as the isArray one. Maybe have a special formatTypedArray too - which can truncate really long typed arrays.

Of course you'd need to add some good tests to ensure good coverage of this, and add guards to make sure engines without typed arrays do not blow up when trying to reference them. Tests for the inspect util are found in test/utilities.js#l355-375

@lucasfcosta
Copy link
Member

Hi @keithamus, I'm planning on tackling this one.
Should I raise my PR under 4.x.x or master?

Would you mind if I opened a pull request tagged as [WIP] so you guys can review, approve/disapprove my changes?

@keithamus
Copy link
Member

@lucasfcosta absolutely feel free to raise it under a [WIP] PR! 4.x.x seems like a good branch to put it under.

@lucasfcosta
Copy link
Member

Hi @keithamus, I've done some progress and I'd like to confirm if my PR has acceptable code to solve this before refactoring it.
I've also pointed out some considerations/questions related to this issue.

As I've said before, the PR is still tagged as WIP because I plan on refactoring it after you guys approve its behavior.
Thanks in advance!

keithamus added a commit that referenced this issue Jan 5, 2016
@lucasfcosta
Copy link
Member

Hi @keithamus, this one is done, I think we can close it 😄

@keithamus
Copy link
Member

Ah, thanks @lucasfcosta 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants