Handling Leap Year Issue and more #126
Conversation
NavyStack
commented
Jan 1, 2024
NavyStack
commented
- Explicitly assigning types to variables and parameters in TypeScript.
- Added a token testing process to account for leap years
- Fix missing some node.js dependencies, including "saslprep"
- Fix debounce strings
👷 Deploy request for simple-comment pending review.Visit the deploys page to approve it
|
src/lib/crypt.ts
Outdated
| const YEAR_SECONDS = 60 * 60 * 24 * 365 | ||
| const isLeapYear = (year: number): boolean => { | ||
| return (year % 4 === 0 && year % 100 !== 0) || year % 400 === 0 | ||
| } | ||
|
|
||
| /** Return a date in *seconds from epoch* | ||
| * some number of (input) seconds in the future | ||
| * This is the format JWT wants */ | ||
| export const getExpirationTime = (secondsFromNow: number): number => | ||
| new Date( | ||
| // Expiration date is set at 10s intervals by dividing, then multiplying, by 10,000ms | ||
| // Also, "now" is in milliseconds and output must be seconds, so we are also dividing by 1000 | ||
| // While we could just divide by 10,000 and multiply by 10, let's make | ||
| // each step explicit, so we can reason about what's happening: | ||
| Math.floor((new Date().valueOf() + secondsFromNow * 1000) / 10000) * 10000 | ||
| export const getExpirationTime = (secondsFromNow: number): number => { | ||
| const now = new Date() | ||
| const currentYear = now.getFullYear() | ||
| const yearSeconds = isLeapYear(currentYear) ? 60 * 60 * 24 * 366 : YEAR_SECONDS |
There was a problem hiding this comment.
I really appreciate the effort you put in, here! However, getExpirationTime() just takes a number of seconds in the future. The number of seconds it takes is arbitrary. It only needs to return a number that represents "x number of seconds from now, represented as the number of seconds from Jan 1, 1970"
It is called on line 33 with YEAR_SECONDS, a constant that is "roughly a year hence" but the business needs of the app do not call for a precise calculation of leap year. In fact, leap year calculation introduces unnecessary complexity, which in turn can lead to errors. For instance, line 15 const yearSeconds is never used!
| const toSlug = (str: string): string => { | ||
| return str | ||
| .toLowerCase() // Convert to lower case | ||
| .trim() // Remove leading and trailing spaces | ||
| .replace(/_/g, "-") // Replace underscores with dashes | ||
| .replace(/\s+/g, "-") // Replace all spaces with dashes | ||
| .replace(/[^a-z0-9-]/g, "") // Remove characters that are not lowercase letters, numbers, or dashes | ||
| .replace(/-+/g, "-") // Merge consecutive dashes into a single dash | ||
| } |
There was a problem hiding this comment.
I like the simplification and comments here. House style is to avoid a return statement when there are no local variables nor otherwise any need for a command block (curly brackets).
src/simple-comment-icebreakers.ts
Outdated
| @@ -23,8 +23,8 @@ const reverseSlug = (slug: string, questions: string[]) => | |||
| questions.find(question => isSlugMatch(slug, question)) | |||
|
|
|||
| const fetchAndStoreQuestions = () => | |||
| new Promise<string[]>((resolve, reject) => { | |||
| const currentTimestamp = | |||
| new Promise<string[]>((resolve: (questions: string[]) => void, reject: (reason?: any) => void) => { | |||
There was a problem hiding this comment.
House style is to avoid any and use unknown, but specifying a type in this case is not necessary and introduces complexity.
src/tests/backend/crypt.test.ts
Outdated
| export const getExpirationTime = (secondsFromNow: number): number => { | ||
| const now = new Date() | ||
| const currentYear = now.getFullYear() | ||
| const yearSeconds = isLeapYear(currentYear) ? 60 * 60 * 24 * 366 : YEAR_SECONDS |
| ["hi-IN", "23 जुलाई 2023 को 1:31 pm"], | ||
| ["id-ID", "23 Juli 2023 13.31"], | ||
| ["it-IT", "23 luglio 2023 13:31"], | ||
| ["hi-IN", "23 जुलाई 2023 को 1:31 pm बजे"], | ||
| ["id-ID", "23 Juli 2023 pukul 13.31"], | ||
| ["it-IT", "23 luglio 2023 alle ore 13:31"], |
There was a problem hiding this comment.
Confused about these. These tests pass on my box and in the Github actions. Are they not passing for you?
|
|
||
|
|
||
| // Use a defined JWT_SECRET or a default for testing | ||
| const jwtSecret = process.env.JWT_SECRET || "default_test_secret" |
There was a problem hiding this comment.
Hi @NavyStack
I really appreciate your effort here.
Could you read and acknowledge the Contributor License Agreement before I incorporate any of your code, please?
You don't have to "sign and return it" as indicated at the bottom. Just acknowledge in reply that you have read and agree to it. The idea is that I would like my company to retain complete ownership and autonomy over Simple Comment. Without explicit agreement, it seems sometimes a contribution can lead to misunderstandings about ownership.
Much appreciated!
|
I agree with CLA I noticed an error when I tested on January 1st KST, but it doesn't seem to occur now. Considering this situation, it would be prudent to use the actual JavaScript Intl.DateTimeFormat API to generate the output for each locale and then compare it with the expected format. |
