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

fix: update sveltekit:prefetch mouse detection #2995

Merged
merged 7 commits into from Dec 22, 2021

Conversation

borntofrappe
Copy link
Contributor

dispatches a custom event so that event.composedPath returns an array of objects - fixes #2929

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Dec 6, 2021

🦋 Changeset detected

Latest commit: f06924d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

.changeset/lazy-ways-swim.md Outdated Show resolved Hide resolved
packages/kit/src/runtime/client/router.js Outdated Show resolved Hide resolved
packages/kit/src/runtime/client/router.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Member

I didn't review the PR yet, but please note that the Lint CI job is failing and would need to be fixed

@borntofrappe
Copy link
Contributor Author

I didn't review the PR yet, but please note that the Lint CI job is failing and would need to be fixed

Apologies, but I don't know how to consider the message "No overload matches this call". Is this connected to TypeScript? Unfortunately I need some guidance on this point.

@bluwy
Copy link
Member

bluwy commented Dec 8, 2021

For "No overload matches this call", TypeScript isn't happy that handle_prefetch requires the MouseEvent | TouchEvent param because of the custom sveltekit:handle_prefetch event. But looking at the types for find_anchor, we could simplify the handle_prefetch param as Event. That should fix it.

@borntofrappe
Copy link
Contributor Author

For "No overload matches this call", TypeScript isn't happy that handle_prefetch requires the MouseEvent | TouchEvent param because of the custom sveltekit:handle_prefetch event. But looking at the types for find_anchor, we could simplify the handle_prefetch param as Event. That should fix it.

I see. Using Event seems to fix that particular issue, but pnpm check highlights another one when accessing event.target.

Object is possibly 'null'.ts(2531)

(property) Event.target: EventTarget | null

I've managed to remove the message by returning the function early if event.target is null, but you might have a better solution.

if (!event.target) return;

event.target.dispatchEvent(
  new CustomEvent('sveltekit:trigger_prefetch', { bubbles: true })
);

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM. The failing tests seems to be caused by an issue with amphtml-validator (unrelated), I've made #3022 to fix this.

@bluwy bluwy added the router label Dec 11, 2021
trigger_prefetch(event);
// ensure event.composedPath() in find_anchor(event) returns an array of EventTarget objects
event.target?.dispatchEvent(
new CustomEvent('sveltekit:trigger_prefetch', { bubbles: true })
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if something like sveltekit:mousestop might be a better name for this? What do you guys think?

Copy link
Member

@bluwy bluwy Dec 13, 2021

Choose a reason for hiding this comment

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

Either works for me, but it does looks nicer when used in the addEventListener call below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lean toward sveltekit:trigger_prefetch as well.

Copy link
Member

Choose a reason for hiding this comment

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

My instinct is to look for an alternative solution altogether — it seems weird to dispatch a custom event just to indirectly call our own function, no?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I realised there's a deeper problem here. The composedPath stuff was introduced because recursively checking parentNode to find an <a> element doesn't work if the <a> in question is in shadow DOM.

The solution here dispatches a custom event on e.target, but the composedPath of the custom event won't include the shadow <a> in certain cases. Demo here — basically, if you have a custom element with an <a> in the shadow DOM like this...

customElements.define(
  'fancy-a',
  class extends HTMLElement {
    constructor() {
      super();

      let a = document.createElement('a');
      a.href = this.getAttribute('href');
      a.innerHTML = '<slot></slot>';

      let shadowRoot = this.attachShadow({ mode: 'open' });
      shadowRoot.append(a);
    }
  }
);

...then whether or not the <a> will be included in the composedPath depends on whether the original event happened to happen within an element in light DOM inside the custom element.

I'm not exactly sure how to solve this, or if it's even possible. We might have to just say that prefetching won't work with shadow DOM.

Copy link
Member

Choose a reason for hiding this comment

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

That does look a bit risky and potentially more resource intensive. I wonder if it would be fine to not support sveltekit:prefetch in web components at all, the usecase would be rare. And also that normal link clicks should already work in web components.

Copy link
Member

Choose a reason for hiding this comment

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

I would be totally fine dropping support for prefetch with web components

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the pragmatic side here. I don't think that dispatching a custom element with an explanation why this is needed is bad nor does it clutter the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem dispatching event is that Rich showed it doesn't work all the times with custom elements. I share the sentiment from @bluwy that the issue is becoming rather cumbersome.

Copy link
Member

Choose a reason for hiding this comment

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

Very well, if we're okay only having partial support for shadow DOM then we can merge this — thanks. Very annoying that the existence of shadow DOM forces us to jump through these ridiculous hoops in the first place!

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@bluwy bluwy changed the title Event composed path fix: update sveltekit:prefetch mouse detection Dec 19, 2021
@Rich-Harris Rich-Harris merged commit 3acdf93 into sveltejs:master Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sveltekit:prefetch not working
5 participants