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 AbortSignal #28

Merged
merged 14 commits into from
Jun 23, 2022
Merged

Add support for AbortSignal #28

merged 14 commits into from
Jun 23, 2022

Conversation

cheap-glitch
Copy link
Contributor

Resolves #26
Blocked by jsdom/jsdom#3347

@cheap-glitch cheap-glitch marked this pull request as draft March 21, 2022 11:01
index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

It might be because of the jsdom PR, but this PR is missing the only test change I'd expect:

  • accept a signal

readme.md Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
index.ts Show resolved Hide resolved
Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Let's merge this without waiting for jsdom. You can:

  1. manually make the jsdom changes in your node_modules
  2. ensure that the tests pass
  3. mark these tests as test.serial.skip
  4. commit
  5. ...
  6. profit

test.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@cheap-glitch
Copy link
Contributor Author

manually make the jsdom changes in your node_modules

Tried it but it's really tricky, there are some JS files generated from .webidl files and they're a mess, I have no idea what to change to make it work…

@fregante
Copy link
Owner

Unblocked by jsdom v20 🙂

Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Fin-ally!

}
},
};
Array.prototype.forEach.call(base, element => {
Copy link
Owner

Choose a reason for hiding this comment

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

Note to self: Before releasing this PR, I'll likely replace ArrayLike with Iterable so this can be a plain for-of (separate PR)

index.ts Outdated
Comment on lines 173 to 175
}, {
once: true,
});
Copy link
Owner

Choose a reason for hiding this comment

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

This feels like a micro-optimization that the engine can take care of since abort only happens once

Suggested change
}, {
once: true,
});
});

Copy link
Contributor Author

@cheap-glitch cheap-glitch Jun 23, 2022

Choose a reason for hiding this comment

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

I added this because it was recommended by the Node.js docs, but I guess this is a browser package so it's probably superfluous 😅

@fregante fregante marked this pull request as ready for review June 23, 2022 15:28
@fregante fregante merged commit 4f3d7ea into fregante:main Jun 23, 2022
@fregante
Copy link
Owner

It's out! Hope to see this used in RG soon, so we can test it out in production

https://github.com/fregante/delegate-it/releases/tag/v4.0.0

@cheap-glitch cheap-glitch deleted the support-signal branch June 23, 2022 17:38
internalController.abort();
});
} else {
listenerOptions.signal = internalController.signal;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the delegated options be registered without any signal instead, as the ledger system can reuse an existing listener for multiple delegations ? Otherwise, aborting one of them would abort the native listener shared between them and so abort them all.

Copy link
Owner

Choose a reason for hiding this comment

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

oof, you sounds right, but the tests pass. Are the tests broken?

From what I understand, you're saying that the ledger cannot deduplicate calls and therefore it has become useless/broken, but I think we have tests specifically to avoid duplicate registrations.

Can you suggest a piece of code that fails with v4 but passes with v3 to make sure we're talking about the same thing?

Copy link
Owner

@fregante fregante Jul 8, 2022

Choose a reason for hiding this comment

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

Ah I see what you mean. I think:

delegate(a, b, c, mysignal)
const controller = delegate(a, b, c);

controller.abort()

Currently the second call is deduplicated, so the listener is only registered once. This has one important consequence:

  • the second call does not have any link to the previous listener, so calling abort does nothing

I think this happens any time, regardless of mysignal actually. 😟

Copy link
Owner

@fregante fregante Jul 8, 2022

Choose a reason for hiding this comment

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

Native events, like us, ignore the signal when deduplicating listeners:

controller = new AbortController()
controller2 = new AbortController()
document.addEventListener('click', console.log, {signal: controller.signal})
document.addEventListener('click', console.log, {signal: controller.signal})
document.addEventListener('click', console.log, {signal: controller2.signal})
document.addEventListener('click', console.log)

This only register's one listener.

The funny thing is that controller2.abort() does nothing while controller.abort() removes the event. Tested in both Safari and Chrome.

This means we're matching the native behavior. The only difference is that we explicitly return an AbortController that does nothing.

Copy link
Owner

Choose a reason for hiding this comment

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

Neat. Not only do native events ignore the successive signals, but ignore future calls altogether!

document.addEventListener('click', console.log, {once: true})
document.addEventListener('click', console.log)

The handler is called once. That's it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, delegating different selectors does not share the ledger, so fine indeed.

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.

Use abort controllers to remove events
3 participants