Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Client-side routing for
<form method="GET">
#7828Client-side routing for
<form method="GET">
#7828Changes from 7 commits
664f49c
a3dc507
17f5d23
061b313
9fa14d5
4aaba34
4239e6e
334d9d9
86a3f87
1cde2e1
bcab9ed
2af026d
5cb712f
2172fae
7621f7d
2536be1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a remnant of my previous suggestion. Github didn't let me include this line in the selection because there was a deleted line in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, we also need to delete some of the empty line or lint would fail, removed in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, probably shouldn't pretend nullable things aren't possibly null and should just handle null in the function or return early
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just TypeScript being dumb though, no? Could
event.target
ever actually benull
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not null in this case though, we know it exists. The types don't, that's why they play it safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is considered premature optimization already, but I thought it might be worth a suggestion.
I'm not a benchmark expert, but I created this (probably unrealistic) benchmark which hints that
closest
+contains
(which we need anyway) might be faster than the while loop:https://measurethat.net/Benchmarks/Show/22320/0/find-anchor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've no doubt that's true, but unfortunately
closest()
doesn't work, because it can't find elements in parent shadow DOM — yet another reason adding it to the web platform was a mistakeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(incidentally,
element.closest(selector)
will returnelement
if it matchesselector
, so if this did work — if we decided not to bother with shadow DOM, on the basis that the wounds are self-inflicted — we could dispense with the firstif
block)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, makes sense. I didn't thought about that.
I included this
element.nodeName.toUpperCase() !== 'A'
check because it was so much faster than.closest('a')
on anHTMLAnchorElement
in my tests.