dependencies: Upgrade JavaScript dependencies #28743
Conversation
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
| @@ -99,7 +99,7 @@ | |||
| "@types/lodash": "^4.14.172", | |||
| "@types/micromodal": "^0.3.3", | |||
| "@types/minimalistic-assert": "^1.0.1", | |||
| "@types/node": "^20.9.0", | |||
| "@types/node": "20.11.5", | |||
There was a problem hiding this comment.
| const active_modal_id = CSS.escape(`${CSS.escape($micromodal.attr("id") ?? "")}`); | ||
| if (active_modal_id === `${CSS.escape(modal_id)}`) { | ||
| Micromodal.close(`${CSS.escape($micromodal.attr("id") ?? "")}`); | ||
| const active_modal_id = CSS.escape(CSS.escape($micromodal.attr("id") ?? "")); |
There was a problem hiding this comment.
It doesn’t make any sense to double-CSS-escape something; there has to be something wrong here…
There was a problem hiding this comment.
Quite so. @sahil839 do you know what the story with this code is?
There was a problem hiding this comment.
The code was added in #25512 by Aman. We do need escaping here as suggested by Anders.
There was a problem hiding this comment.
Huh? When did I suggest that we need escaping here, let alone double-escaping?
Escaping is not some magic fairy dust that you sprinkle everywhere to make your code pretty. It has a specific purpose. Its purpose is to correctly insert an inner string into an outer string in some language, such that, when the outer string is later parsed in that language, the inner string will be recovered as-is and won’t be reinterpreted as extra syntax in the outer language.
For example, if the outer language is CSS selectors, we have
» id = "foo.bar"
» selector = `#${id}`
"#foo.bar" // wrong: selects element with id `foo`, class `bar`
» selector = `#${CSS.escape(id)}`
"#foo\\.bar" // right: selects element with id `foo.bar`But active_modal_id is not a CSS selector. It is not an outer language that’s going to be re-parsed to recover an inner string. Escaping is incorrect here.
And double-escaping is wrong even in the CSS selector case:
» selector = `#${CSS.escape(CSS.escape(id))}`
"#foo\\\\\\.bar" // wrong: selects element with id `foo\.bar`The only time when a string should be escaped twice is when it’s going to be parsed twice in a nested way (for example, a URL query parameter in an HTML attribute needs to be URL-encoded and then HTML-escaped so that it can be correctly HTML-parsed and then URL-parsed).
There was a problem hiding this comment.
Oops, very sorry. I actually meant to write "do not need", should have read the comment once after posting it.
Again sorry for the inconvenience.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
andersk
force-pushed
the
upgrade-dependencies
branch
from
00a1a20
to
0c485cf
Compare
| @@ -869,7 +869,7 @@ export function sender_info_for_recent_view_row(sender_ids: number[]): SenderInf | |||
| const senders_info = []; | |||
| for (const id of sender_ids) { | |||
| // TODO: Better handling for optional values w/o the assertion. | |||
There was a problem hiding this comment.
This TODO probably can be deleted, right?
There was a problem hiding this comment.
Hmm, maybe this should be get_user_by_id_assert_valid; @sahil839 thoughts on that detail?
There was a problem hiding this comment.
I cannot understand what optional values does the TODO statement talk about, so not sure about that but we can use get_user_by_id_assert_valid here. Though this also works fine as we already create user objects for inaccessible message senders when processing message.
|
Merged, as both of the above comments are probably to be resolved in a follow-up PR by @sahil839. |
No description provided.