Better error message when the scrollBehavior selector is invalid

[theprojectsomething]

What problem does this feature solve?

Currently if you pass a hash id with unescaped CSS special characters /[!"#$%&'()*+,\-./:;<=>?@[\\]^{|}~]/ to a scrollBehavior selector, execution will fail:

'XXX' is not a valid selector

Because of this url-styled hashes (e.g. "#one/two" or "#main/secondary"), both valid ids in HTML5, cannot be passed directly from the to.hash or from.hash parameters.
This is because behind the scenes document.querySelector, which relies on CSS character compliance, is used in all but a very small number of circumstances (i.e. /^#\d/). This feature proposes broadening the use case for document.getElementById to all selectors that have the appearance of an id, specifically ungrouped selectors starting with a hash (i.e. /^#[^, ]+$/)

What does the proposed API look like?

API doesn't change, except the following would work:

scrollBehavior(to, from, savedPosition) {
  return {
    selector: to.hash,
  }
}
// where:
window.location = '/one/two#three/four';

[posva]

After further research, I'm realizing that trying to be smart and detect valid id selectors is a bad idea because there are cases where it won't be possible. Adding support for hashes starting with a number was a mistake and has been removed in v4. So instead I think we should do the following

• Deprecate selector in favour of el (Element | string like new Vue({ el }) and app.mount(el))
• Show a deprecation notice when the selector matches /^#\d pointing the user to use the el option instead
• Show a warning if el is a string, starts with a # and fails with querySelector pointing out to use document.getElementById(<theid>) or to escape the selector and point to a resource.
• A version will be updated at https://github.com/vuejs/vue-router-next/blob/master/src/scrollBehavior.ts#L69-L99

[theprojectsomething]

Hi @posva thanks for coming back. Couple of notes ...

It's been a while, but I think the gist/context of the original PR was that scrollToPosition was being used internally by the router to replicate/implement scrolling to the location/href hash on route change. I'm not sure if that's still the case (or maybe I'm wrong - was it ever the case??).

Given that the hash is meant to map to an id (or technically also a [name]) within the document, I would have expected this concept to be a first class citizen within the logic. Using querySelector breaks this, unless there is intervention by the developer (which will be required for everything but hardcoded anchors).

I like the new error message, it will definitely help debug. If it's still the case, can I suggest that it's also noted in the documentation that querySelector is being used under the hood to match url hashes?

[posva]

Vue Router doesn't replicate the hash scrolling from the browser, we use scrollTo to scroll to the position passed by the user and provide selector for convenience.

Note the fragment of the url has multiple use cases, anchoring to an id of the page is the most common one but only one of them.

I think it would be better to deprecate selector and introduce a new el option. I updated the comment above, let me know what you think!

[theprojectsomething]

Nice updates! These all make a lot more sense I think - more clarity, less edge-cases 😁

Sorry for the confusion around the hash scrolling, I had quickly looked through my own code / fix to see where the PR had originated from and that seemed to be the case.

Note there is reference to the scroll-to-hash behaviour in the docs, specifically it mentions passing to.hash to selector but I think if it is clear that this should be passed as an el instead then it's a perfectly good outcome. Cheers!

[posva]

Glad to hear it makes more sense! The docs should indeed note that querySelector is used behind the scenes and that it doesn't always work.

to be clear it would be el: document.getElementById(to.hash.slice(1)), but that would work without escaping the hash!

Another possibility would be to always use an document.getElementById if el.startsWith('#') and write a specific warning if no element is found that shows that getElementById is used when el starts with # as well as pointing out that we can use el: document.querySelector(to.hash) (which is less boilerplate than the one with slice). I think this option makes the most sense in the end because it will still allow to just do el: to.hash, will work in most scenarios without the user needing to escape the CSS selector and will still warn when failing providing a way to make it work no matter what.

I will write an RFC for this, it's worth one

[posva]

posted vuejs/rfcs#176