vue-router
vue-router copied to clipboard
Better error message when the scrollBehavior selector is invalid
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';
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 ofel
(Element | string
likenew Vue({ el })
andapp.mount(el)
) - Show a deprecation notice when the selector matches
/^#\d
pointing the user to use theel
option instead - Show a warning if
el
is a string, starts with a#
and fails withquerySelector
pointing out to usedocument.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
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?
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!
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!
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
posted https://github.com/vuejs/rfcs/pull/176