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

Use modern triple equals and change to js module #59

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use modern triple equals and change to js module #59

wants to merge 3 commits into from

Conversation

Nikaoto
Copy link

@Nikaoto Nikaoto commented Oct 30, 2018

Replaced:

  • every != with !==
  • every == with ===

Also, changed to a js module so it can be used with webpack and other bundlers.

@@ -339,11 +339,11 @@ window.Whammy = (function(){
}

function toFlatArray(arr, outBuffer){
if(outBuffer == null){
if(outBuffer === null){
Copy link

Choose a reason for hiding this comment

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

I happened to read through this and believe this change introduces a subtle bug. A few lines above in L334, toFlatArray() is called, but without a second argument, i.e. outBuffer will be undefined here. The expected behavior before was that in this case, a new array will be created, and indeed undefined == null in JavaScript (the way it was before your change). Now the test is strict and fails therefore. As a consequence no array is created when outBuffer is undefined, which leads to an error. Maybe it would be best to just test if (!outBuffer) { or if (outBuffer === undefined) {?

Copy link
Author

Choose a reason for hiding this comment

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

That's a correct remark. I should've noticed that when changing it.

In this case, however, outBuffer is never really undefined since toFlatArray is only used once on L334 outside the function:

// Line 330
		//output as blob or byteArray
		if(outputAsArray){
			//convert ebml to an array
			var buffer = toFlatArray(ebml)
			return new Uint8Array(buffer);
		}else{
			return new Blob(ebml, {type: "video/webm"});
		}

And the variable ebml, defined on L297 can't become undefined since the only changes made to it are a few push methods. So, in this case, my changes don't introduce fatal bugs, but they're still dangerous considering future modifications made to the code might have an undefined array be passed to toFlatArray.

What's more, there's a better way to export whammy as a module for nodejs without having the browser give an annoying error for accessing a non-existent global module.exports.

I'll fix both issues as soon as possible.

Copy link
Author

Choose a reason for hiding this comment

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

This PR actually fixes the second issue I wrote about: #46

Copy link

Choose a reason for hiding this comment

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

And the variable ebml, defined on L297 can't become undefined since the only changes made to it are a few push methods

While it is true that the variable embl can't become undefiend, it is actually the second parameter to toFloatArray() (the parameter outBuffer) that I worry about. If toFloatArray() is called in L334 without this second argument, only embl is passed in, which in turn is available as argument arr within toFloatArray(). The second parameter (outBuffer) is undefined in this situation and this results in an error if the arr contents are non-objects and get pushed to outBuffer.

The other changes look good to me though and the PR itself is certainly useful.

Copy link
Author

Choose a reason for hiding this comment

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

It seems to have slipped my mind. outBuffer is definitely undefined when no second argument is passed to toFloatArray.

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.

2 participants