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 isSameNode method #44

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

Conversation

wclr
Copy link

@wclr wclr commented Feb 27, 2021

Add isSameNode mehtod

Spec: https://dom.spec.whatwg.org/#dom-node-issamenode

MSN: https://developer.mozilla.org/en-US/docs/Web/API/Node/isSameNode


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@garyb
Copy link
Member

garyb commented Feb 27, 2021

I think maybe this doesn't need to be in Effect, as it's not something that can change over time, is it?

Also, if you happen to open any PRs/issues on this library in the future, please link to the DOM living standard, as per the PR request template, instead of MDN - not everything in MDN is part of the standard.

@wclr
Copy link
Author

wclr commented Feb 28, 2021

I think maybe this doesn't need to be in Effect, as it's not something that can change over time, is it?

Well, isEqualNode uses Effect, so I added this for this method. Though isEqualNode seems to be really side-effective, as it is about comparing of values that need to be retrieved from DOM and that can change over time for the same node, and in case of isSameNode that is about comparting this, it should be constant and safe to access over time.

So seems isSameNode should not use Effect. Should something else be considered, what would you say?

, please link to the DOM living standard

I've added the link.

@garyb
Copy link
Member

garyb commented Feb 28, 2021

Yeah, I think isEqualNode does need to be in effect, since a node can be mutated in such a way that means calling isEqualNode at different times with the same two argument will give a different answer.

isSameNode is always going to return the same value for its arguments, it has no effect on the larger DOM, and the spec doesn't say anything about it throwing errors - that would be the only other concern that would make it effectful - so I do think it's safe to take it out of Effect. 🙂

@wclr
Copy link
Author

wclr commented Feb 28, 2021

Ok, I've changed it.

Is changelog entry needed, are there guidelines on how to fill it?

@wclr
Copy link
Author

wclr commented Jul 22, 2021

Is it ok to merge? Or is it to be merged with the next release?

@thomashoneyman
Copy link
Contributor

Sorry for the late response, @wclr. You do need to update the changelog (CHANGELOG.md). This would be added as a 'new feature' under the '[UNRELEASED]' section, something like:

New features:
- Added `isSameNode` function (#44 by @wclr)

and I can merge and release it.

sigma-andex pushed a commit to working-group-purescript-es/purescript-web-dom that referenced this pull request Apr 14, 2022
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.

3 participants