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

Implementation of set.equals #3097

Merged
merged 5 commits into from
Apr 19, 2021
Merged

Implementation of set.equals #3097

merged 5 commits into from
Apr 19, 2021

Conversation

mvaligursky
Copy link
Contributor

extracted from #2990

@mvaligursky mvaligursky self-assigned this Apr 16, 2021
@mvaligursky mvaligursky requested a review from a team April 16, 2021 16:32
@willeastcott
Copy link
Contributor

I can't find this function in the JavaScript standard library:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

If Set#isEqual is not in the JS language spec, then this is not a polyfill.

@mvaligursky
Copy link
Contributor Author

it's a polyfill from the future when JS implements this function ;)

do we have some better place to add these? Perhaps we could create a new folder for these. Or just add it to "/core"?

@willeastcott
Copy link
Contributor

My feeling is that if we are not polyfilling a legitimately existing JS language feature/function, we shouldn't screw with standard library classes, poking in our own stuff. You could stick a utility function in core\set-utils.js or something? But yeah, operate on set instances rather than inject stuff into Set.

src/index.js Outdated Show resolved Hide resolved
@Maksims
Copy link
Contributor

Maksims commented Apr 17, 2021

Feels like it could be better named, consistent with Vec3 for example: equals.

@mvaligursky mvaligursky changed the title Implementation of Set.isEqual Implementation of set.equals Apr 19, 2021
@mvaligursky mvaligursky merged commit fc4dace into master Apr 19, 2021
@mvaligursky mvaligursky deleted the mvaligursky-set-isequal branch April 19, 2021 08:41
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.

None yet

3 participants