-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
url: fix lint and deopt issues #5300
Conversation
The deopt issues arose from the use of const in specific situations that v8 does not fully support yet.
@mscdex can you shed some light on what specifically is not supported to cause the deopt for using const? |
@evanlucas I'm not enough of a v8 guru to understand. At first I thought it had to do with using a ternary expression in the value for |
Thanks |
CI is all green again. |
LGTM |
LGTM. Maybe add |
Also, since this fixes CI, I'd be all for landing it in an expedited fashion. |
LGTM |
@Trott agreed we want to land to fix the CI. Asked James to review since he was one of the original reviewers and since we have his LGTM now I'd say we're ready to land |
@Trott do you have cycles to land ? If not I'll try to fit it in this afternoon. |
Landed in 7d1d3a6 |
this does not land cleanly on LTS, would someone be willing to backport it? |
@thealphanerd I'm guessing it isn't landing cleanly because #4892 has not landed yet. So if that can't land right away, maybe this can also be delayed until that lands first? /cc @mscdex in case he wants to go ahead and backport. I'm not sure whether there's a reason to push this ahead of that other PR, though. |
@Trott The only thing to be backported from this PR if the original url perf commit isn't also backported, are merely the few changes made to |
I'm moving the label to don't land for right now. We can change it back if #4892 ever lands |
The deopt issues arose from the use of const in specific situations that v8 does not fully support yet.