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

add support for typed arrays #5905

Merged
merged 1 commit into from Dec 18, 2018
Merged

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Dec 10, 2018

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @kurkle.

helpers.isArray is used a lot even outside this project, so we need to be sure that returning true for all arrays (basic and typed) will not break existing code. Is there any use case where we don't want to handle both the same way?

.eslintrc.yml Outdated Show resolved Hide resolved
test/specs/helpers.core.tests.js Outdated Show resolved Hide resolved
src/helpers/helpers.core.js Outdated Show resolved Hide resolved
src/helpers/helpers.core.js Outdated Show resolved Hide resolved
@kurkle kurkle force-pushed the typed-arrays branch 3 times, most recently from 0607db9 to 38fd7bd Compare December 12, 2018 14:44
@kurkle
Copy link
Member Author

kurkle commented Dec 12, 2018

There is one place, where I get the idea this could break outside code
(in src/core/core.tooltip.js):

// Helper to push or concat based on if the 2nd parameter is an array or not
function pushOrConcat(base, toPush) {
	if (toPush) {
		if (helpers.isArray(toPush)) {
			// base = base.concat(toPush);
			Array.prototype.push.apply(base, toPush);
		} else {
			base.push(toPush);
		}
	}

	return base;
}

Here, if base would be typed array, Array.prototype.push.apply would not work. This is never the case inside this lib though (and base is not actually tested with isArray anyway).

So if someone tested with helpers.isArray and based on that tried to push, it would break. There's no push/pop in typed arrays.

@simonbrunel
Copy link
Member

I didn't know that typed arrays didn't support push/pop. I don't think that's an issue because it would fail only if this library or external plugins try to alter the size of a typed array after checking helpers.isArray(). In that case, the code expects a resizeable array, meaning that it's either an internal bug (use of the wrong structure) or a bad user input.

If that's the only difference, I think it's safe to merge (though, not fully confident).

@etimberg @nagix do you think of use cases that this PR would break?

simonbrunel
simonbrunel previously approved these changes Dec 12, 2018
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @kurkle

@@ -1,4 +1,8 @@
parserOptions:
ecmaVersion: 5 # don't rely on default, since its changed by env: es6
Copy link
Member

Choose a reason for hiding this comment

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

It should be fine if our tests use es6 features, we only don't want our sources to use es6 feature because we don't want to integrate babel in v2.

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

Successfully merging this pull request may close these issues.

None yet

3 participants