-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
Reduce calling scp, cache result #3025
base: dev
Are you sure you want to change the base?
Conversation
I think just adding a debounce might be a better solution:
Tested it locally and it works, just not sure what the best delay amount would be, this was a test w/ a 3 second delay just to ensure the code is working..itd probably be better at like ~1 second also we should add a spinner next to the input so the user knows theres a request being sent: |
Not sure if we want to solve the same problem ;). I'd go for trying to get suggestions as early and fast as possible and narrow them down as fast as possible, therefore it seems counterintuitive to do 'debouncing', but more natural to filter stored results. |
So based on the pr description, I thought we were trying to reduce the number of requests sent from the frontend to the backend as the user types.
The debounce solution is the standard go to way of preventing sending a request as a user types until they are done typing. I think implementing caching here is setting up for problems down the road. There's an old joke in programming:
|
... but the cited sentence has some more words to it ;). I'm totally fine with alternative solutions taking other priorities in server-load and user-experience, but I guess here I'd appreciate an eye on this particular approach - especially, as pointed out, since cache invalidation is not the easiest thing to do. Feel free to point out gaps I might have left. The approach taken just uses a state-model representing if and for which characters a request is on its way. |
... one thing to add: I guess there could be different solutions 'right' for 'normal' and for 'contest' logging. Based on your screenshots you seem to focus on 'normal'-logging, while I started implementing this for contest-logging. |
I definitely see where you're coming from, and caching is a valid way of solving the stated problem. Both solutions solve the problem, and can even work in tandem together. With that said, in my personal experience, it would best to start with the debounce method at ~300-500ms delay (which isn't even noticeable to the end user, it's just enough to prevent multiple calls if the user mistypes and quickly fixes). The main reasons to start with debounce over cache are:
If we implement debounce and *you're still not happy with how it solves the problem, then I think it'd be the appropriate time to look at integrating some type of caching. I hope this clears up where I'm coming from and the best practice for order of operations on a problem/solution like this. |
For SCP you want to get results as fast as possible this obviously is different for the QSO box fields some debounce on those would be a great idea. |
This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected |
Thanks for trying this out and pointing me to issues where '0' is part of the callsign. What is the convention about the use of '0' or 'Ø' in callsigns in Cloudlog? |
- fix '0' - improvements in data handling - filter allows prefixes
There you go. Thank you for input and testing, appreciate it.
|
@magicbug, please let me know if there's anything which prevents merging. |
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 still dont like this change, but i can add more of my changes if this get's accepted.
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.
In JavaScript, whether to use let, const, or var for variable declarations depends on the specific use case and the scope where the variable is needed.
var: This is the oldest keyword for declaring variables, existing since the beginning of JavaScript. Variables declared with var are function-scoped or globally-scoped if declared outside of a function. They can be re-declared and updated. However, var has some pitfalls, such as hoisting, which can lead to bugs, especially in large codebases.
let: Introduced in ES6 (ECMAScript 2015), let provides block-scoped variables. Unlike var, variables declared with let are limited in scope to the block, statement, or expression where they are used. let allows you to update the variable but not re-declare it within the same scope. This reduces the risk of bugs compared to var.
const: Also introduced in ES6, const is used to declare variables whose values are not intended to change. const provides block-scoping similar to let. The key difference is that once a variable is assigned with const, its value cannot be reassigned. Attempting to do so will result in a runtime error. However, note that if a const variable holds an object or an array, the object or array's contents can still be modified (the variable's reference to the object or array cannot change).
Best Practices
Prefer const: Use const by default for all of your variable declarations if you know their values will not change. This makes your code easier to reason about since you know these variables won't be reassigned later in your code.
Use let for variables that change: If you have variables that need to change, like counters in a loop, or values that get reassigned, use let. This signals to others that this variable will be reassigned.
Avoid var: Due to its function scope and hoisting behavior, var can lead to confusion and bugs, especially in complex codebases. Modern JavaScript codebases tend to avoid var in favor of let and const for clearer scoping rules.
In conclusion, while it's not a rule to "always use let," the modern practice leans towards using const by default and let when necessary for variables whose values need to change. Avoiding var is generally considered good practice in contemporary JavaScript development.
assets/js/sections/contesting.js
Outdated
@@ -8,6 +8,11 @@ $(document).ready(async function () { | |||
setRst($("#mode").val()); | |||
}); | |||
|
|||
var scp_data = { |
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 should be let, i will net let it go.
assets/js/sections/contesting.js
Outdated
highlight(call.toUpperCase()); | ||
} | ||
}); | ||
if ( scp_data.request == "" || ! call.startsWith(scp_data.request) ) { |
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 needs to be ===
assets/js/sections/contesting.js
Outdated
callsign: call | ||
}, | ||
success: function (result) { | ||
var call_now = $("#callsign").val().toUpperCase(); // Might have changed while running the query... |
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 should actually be let
,
assets/js/sections/qso.js
Outdated
@@ -1,4 +1,9 @@ | |||
var lastCallsignUpdated="" | |||
var scp_data = { |
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.
another let
Please read up on javascript best practices before refusing changes that catch the code up to javascript best practices. We should ALWAYS use We should ALWAYS use |
Feel free not to use the // On Key up check and suggest callsigns
$("#callsign").keyup(() => {
const call = $(this).val().toUpperCase();
const cleanedCall = call.replace('0', 'Ø');
// Exit early if the users starts typing a completely new call
if (scp_data.previousRequest && !call.startsWith(scp_data.request)) {
scp_data.previousRequest.abort();
}
if (call.length >= 3) {
if (scp_data.request === "" || !call.startsWith(scp_data.request)) {
scp_data.request = call;
scp_data.data = [];
scp_data.previousRequest = $.ajax({
url: 'lookup/scp',
method: 'POST',
data: {
callsign: call
},
success: function (result) {
const currentCall = $("#callsign").val().replace('0', 'Ø').toUpperCase();
if (currentCall.startsWith(call)) {
scp_data.data = result.split(" ");
$('.callsign-suggestions').text(filterCallsignList(currentCall, scp_data.data));
highlight(currentCall);
}
},
complete: function () {
scp_data.previousRequest = null;
}
});
} else {
// Filter the already obtained list:
$('.callsign-suggestions').text(filterCallsignList(cleanedCall, scp_data.data));
highlight(cleanedCall);
}
const qTable = $('.qsotable').DataTable();
qTable.search(call).draw();
}
else {
$('.callsign-suggestions').text("");
}
}); It's inarguably cleaner than what've you've provided in your PR. |
I'll be honest the convention in Cloudlog should be Ø but.. it varies perhaps something for improvement. Can confirm that it now works, but we have lost the ability to just type the suffix and show matches based on that which is super handy with the SCP box |
- server side matches anywhere, thus replace `startsWith` by `includes`, - callsigns *should* be with stroked-0, improving consistency here, - code restructuring (reduce nesting a lot), - isolating code, same codebase for qso.js and contesting.js, could be put in a 'lib'-like file
Sounds like s.th. to work with and to have incremental improvement :). Now replacing 'Ø' by '0' in
Thanks for pointing that out, haven't observed nor used that up to now (I seem not to remember suffixes without prefixes :D), so now consistently switching to Is there a library-like js-file to have one place for code(-pieces) like this? I have separated the 'functional' code from its specific application to a keyup, so both, Edit: I might add, that the check for cache validity got inverted, you'll directly get to the updated expression with de Morgan. |
For callsign suggestions in logging, Cloudlog requested a list from the server with every additional character typed, from a minimum of 3 on.
This PR reduces calls to the server, by saving and filtering a (matching) list from the server for additional characters typed.
Example: You typed 'DJ3' and got a list of all callsigns. Now you add 'C' ('DJ3C') and while previously, the server was asked to give a new suggestion list with this PR the already received list is filtered for 'DJ3C'.
If the original request does not match the typed callsign anymore (Example: You replace the '3' in the previous example by a '4'), a new request is made.
Included in this PR is contest-, as well as normal-logging.