Warning Header (Closes #205) #394

Merged
snedadah merged 19 commits from warning-header into main 6 months ago
Collaborator

Update:
In order to solve the issues discussed below, we decided to put the warnings in a json file, since they can be easily imported into a javascript file and webpack automatically bundles them with the client side app, so no static props is needed.


To get the warning data, since it is something that needs to be on all pages, it makes sense for the component to go in the app.js file (similar to the nav bar). However, next.js has a cumbersome issue that it does not support getStaticProps in the app.ts file (and getStaticProps only works in page files), thus we have no way of requesting the warning data easily in the <WarningHeader> Component. (https://github.com/vercel/next.js/discussions/10949)

Here is my solution:

  • Request the warning data on the client side through the use of an api.
    • I made a warning api (/api/currentWarning) which sends a json of the current warning
    • Advantage: warnings will always be up to date since it is recalculated on every request
    • Advantage: It can potentially incorporate with other CSC services who might need to know about the warning (eg linktree?)
    • Disadvantage: can get expensive if we have a lot of warnings, to fix this I can cache the current warning and only re-read the files every 24h if this is a problem, but if we don’t have that many warnings I think it should be fine
    • Disadvantage: listed below:

The current problem with what I implemented is that it doesn’t build in the production environment:

  • The way we build the website, when we call “next export” that disables any api endpoints.
  • According to this https://github.com/vercel/vercel/discussions/6551, if we want to allow api endpoints, we have to just do “next build”. This has implications that the site won’t be completely static anymore, but the other solutions also have similar problems ( though the GitHub post says that next will still optimize for static with only next build).

Another advantage of allowing api’s is that maybe in the future we will implement other features where an api might be useful.

If you want to try it out, my branch works fine locally (when just running npm run dev or npm run build”)

Other possible solutions:

  • Use getInitialProps inside _app.js, but this has the downside that it will “disable Automatic Static Optimization in pages without Static Generation.” (https://nextjs.org/docs/advanced-features/custom-app), which I believe will slow the whole site down, but, we will be able to still use the old build command I think, I am not sure of the full effect of this.

    • However, implementing this would also be relatively simple.
  • Add the warning request to the getStaticProps of every single page, possibly through some wrapper component around every single page, this has the disadvantage that we need to do a lot of refactoring of all the pages of the site and cant use the intended “_app.ts” wrapper.

  • Add warning header only to the homepage (or maybe one or two other important pages).

    So how should I handle this?

Update: In order to solve the issues discussed below, we decided to put the warnings in a json file, since they can be easily imported into a javascript file and webpack automatically bundles them with the client side app, so no static props is needed. __________ To get the warning data, since it is something that needs to be on all pages, it makes sense for the component to go in the app.js file (similar to the nav bar). However, next.js has a cumbersome issue that it does not support getStaticProps in the app.ts file (and getStaticProps only works in page files), thus we have no way of requesting the warning data easily in the \<WarningHeader\> Component. (https://github.com/vercel/next.js/discussions/10949) Here is my solution: - Request the warning data on the client side through the use of an api. - I made a warning api (/api/currentWarning) which sends a json of the current warning - Advantage: warnings will always be up to date since it is recalculated on every request - Advantage: It can potentially incorporate with other CSC services who might need to know about the warning (eg linktree?) - Disadvantage: can get expensive if we have a lot of warnings, to fix this I can cache the current warning and only re-read the files every 24h if this is a problem, but if we don’t have that many warnings I think it should be fine - Disadvantage: listed below: The current problem with what I implemented is that it doesn’t build in the production environment: - The way we build the website, when we call “next export” that disables any api endpoints. - According to this https://github.com/vercel/vercel/discussions/6551, if we want to allow api endpoints, we have to just do “next build”. This has implications that the site won’t be completely static anymore, but the other solutions also have similar problems ( though the GitHub post says that next will still optimize for static with only next build). Another advantage of allowing api’s is that maybe in the future we will implement other features where an api might be useful. If you want to try it out, my branch works fine locally (when just running npm run dev or npm run build”) Other possible solutions: - Use getInitialProps inside _app.js, but this has the downside that it will “disable Automatic Static Optimization in pages without Static Generation.” (https://nextjs.org/docs/advanced-features/custom-app), which I believe will slow the whole site down, but, we will be able to still use the old build command I think, I am not sure of the full effect of this. - However, implementing this would also be relatively simple. - Add the warning request to the getStaticProps of every single page, possibly through some wrapper component around every single page, this has the disadvantage that we need to do a lot of refactoring of all the pages of the site and cant use the intended “_app.ts” wrapper. - Add warning header only to the homepage (or maybe one or two other important pages). So how should I handle this?
snedadah added 7 commits 7 months ago
snedadah requested review from a258wang 7 months ago
snedadah requested review from n3parikh 7 months ago
snedadah requested review from j285he 7 months ago
Owner

Thanks for the detailed research and explaining the tradeoffs, it's really helpful and well done!

Personally, I am hesitant to add an API. Having the website being fully static makes deploying really easy, and also means we don't need to worry about monintoring or making sure something hasn't crashed, has a bug etc.

In terms of alternatives, adding a warning header only on the homepage might be the best approach, but I'm not really sure yet. I think we should discuss it at sitdown this week and see what the rest of the team thinks.

Thanks for the detailed research and explaining the tradeoffs, it's really helpful and well done! Personally, I am hesitant to add an API. Having the website being fully static makes deploying really easy, and also means we don't need to worry about monintoring or making sure something hasn't crashed, has a bug etc. In terms of alternatives, adding a warning header only on the homepage might be the best approach, but I'm not really sure yet. I think we should discuss it at sitdown this week and see what the rest of the team thinks.
snedadah added 3 commits 6 months ago
a3thakra reviewed 6 months ago
--form-invalid: #9f616a;
--warning-background: #dd0014;
--warning-text: #ffffff;
Owner

Make sure to add these colors in our theme palette as well. That way it will show up in the themer and will be totally customizable!

Themer: https://csclub.uwaterloo.ca/themer

Adding a color to the palette should be as easy as adding a string to this array. If that doesn't work, let me know and we can figure something out.

Make sure to add these colors in our theme palette as well. That way it will show up in the themer and will be totally customizable! Themer: https://csclub.uwaterloo.ca/themer Adding a color to the palette should be as easy as adding a string to [this array](https://git.csclub.uwaterloo.ca/www/www-new/src/branch/main/components/Theme.tsx#L18). If that doesn't work, let me know and we can figure something out.
snedadah marked this conversation as resolved
a3thakra reviewed 6 months ago
import styles from "./WarningHeader.module.css";
// Had to add these functions here, since we cannot use import the file from ./lib/events since that uses libraries only on the serverside
Owner

We should probably move these functions to utils in that case, and then make both events and this file import from there

We should probably move these functions to utils in that case, and then make both events and this file import from there
snedadah marked this conversation as resolved
a3thakra reviewed 6 months ago
if (
!startDate ||
!endDate ||
isNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format)
Owner

No need to do all of these checks imo, the parsing library should take care of that.

No need to do all of these checks imo, the parsing library should take care of that.
Poster
Collaborator

The reason I added this check, was that the function getLocalDateFromEST would throw a fatal error if it was called with an invalid date (the site would not even load, at least in development mode)

I'm not sure though, since maybe this is good behaviour to have if the user accidentally made a mistake in one of the dates, they will be sure to know if they try to run the site.

Please let me know what behaviour is prefered?

The reason I added this check, was that the function getLocalDateFromEST would throw a fatal error if it was called with an invalid date (the site would not even load, at least in development mode) I'm not sure though, since maybe this is good behaviour to have if the user accidentally made a mistake in one of the dates, they will be sure to know if they try to run the site. Please let me know what behaviour is prefered?
Owner

yes, we should fail on an invalid state. In general it's better to fail while building than while running.

that being said we if the fatal error wasn't making it obvious that it's happening because of an invalid date, we should add some more information there.

yes, we should fail on an invalid state. In general it's better to fail while building than while running. that being said we if the fatal error wasn't making it obvious that it's happening because of an invalid date, we should add some more information there.
a3thakra reviewed 6 months ago
export function WarningHeader() {
const warning = getCurrentWarning();
return !warning || warning.message == "" ? (
Owner

I think it's safe to assume that if a warning is not null, message will not be ""

I think it's safe to assume that if a warning is not null, message will not be `""`
Owner

Doing input sanitation is good practice in general, but it doesn't give us a lot of benefit in most of our usecases. Input sanitation is more or less required when we are dealing with arbitrary input from random users. I think in our case, we are our own users, and we can sort of figure out what the 'input' should be. That plus code reviews make it hard to accidentally add something that's wrong.

Doing input sanitation is good practice in general, but it doesn't give us a lot of benefit in most of our usecases. Input sanitation is more or less required when we are dealing with arbitrary input from random users. I think in our case, we are our own users, and we can sort of figure out what the 'input' should be. That plus code reviews make it hard to accidentally add something that's wrong.
Owner

on another note, you should use foo == null to do null checks.

!foo will work on falsy values like 0, "" as well, where as foo == null only returns true for undefined and null.

If you are doing any comparison other than null (^) you should always use ===. This avoids some pitfalls of the JS language.

on another note, you should use `foo == null` to do null checks. `!foo` will work on falsy values like 0, "" as well, where as foo == null only returns true for `undefined` and `null`. If you are doing any comparison other than null (^) you should always use ===. This avoids some pitfalls of the JS language.
snedadah marked this conversation as resolved
snedadah added 1 commit 6 months ago
d81a5ae9ab added a new sample warning
a3thakra reviewed 6 months ago
const warning = getCurrentWarning();
return !warning || warning.message == "" ? (
<></>
Owner

you can return null here

you can return null here
snedadah marked this conversation as resolved
a3thakra reviewed 6 months ago
const today = new Date();
let currentWarning: Warning | null = null;
warnings.forEach((warning) => {
Owner

instead of setting the currentWarning, you should use a declarative approach:

const currentWarnings = warnings.filter(warning => ...do the thing to filter by date...);
instead of setting the `currentWarning`, you should use a declarative approach: ```ts const currentWarnings = warnings.filter(warning => ...do the thing to filter by date...); ```
snedadah marked this conversation as resolved
a3thakra reviewed 6 months ago
background-color: var(--warning-background);
padding: calc(5rem / 16);
color: var(--warning-text);
font-size: calc(15rem / 16);
Owner

this is an opinion with no justification whatsoever, so feel free to ignore this if you don't feel the same way, I'm not gonna be offended :)


I like to use even number sizes. So either 14px or 16px would be good.

this is an opinion with no justification whatsoever, so feel free to ignore this if you don't feel the same way, I'm not gonna be offended :) --- I like to use even number sizes. So either 14px or 16px would be good.
Poster
Collaborator

I actually prefer it at 6 and 16 too! (not since I like even numbers, but just in terms of the layout :) )

I actually prefer it at 6 and 16 too! (not since I like even numbers, but just in terms of the layout :) )
snedadah marked this conversation as resolved
Owner

Looks good overall @snedadah! 🎉

Looks good overall @snedadah! 🎉
Owner

Can you also update the PR comment with why we are using JSON? Don't delete what's already there, just add something on top of it so our future selves have some context about why we made this decision.

PS: Your detailed description is really nice!

Can you also update the PR comment with why we are using JSON? Don't delete what's already there, just add something on top of it so our future selves have some context about why we made this decision. PS: Your detailed description is really nice!
snedadah added 3 commits 6 months ago
snedadah added 1 commit 6 months ago
18e1448ec0 adjusted warning css
snedadah added 1 commit 6 months ago
31d72b7265 fixed incorrect date format import
a3thakra reviewed 6 months ago
isNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format), since getLocalDateFromEST fails with invalid dates
isNaN(endDate.getTime())
) {
console.error("WARNING WITH INVALID DATES:", warning);
Owner

I would recommend throwing instead of logging the error. Just logging the error makes us deal with things that aren't defined. If we throw, we don't have to deal with undefined behavior.

I would recommend throwing instead of logging the error. Just logging the error makes us deal with things that aren't defined. If we throw, we don't have to deal with undefined behavior.
snedadah marked this conversation as resolved
snedadah added 1 commit 6 months ago
598ffc20f9 updated warning error behaviour
snedadah changed title from WIP: Warning Header (Closes #205) to Warning Header (Closes #205) 6 months ago
a258wang reviewed 6 months ago
isNaN(startDate.getTime()) || // this checks if the parsed date is not valid (eg. wrong format), since getLocalDateFromEST fails with invalid dates
isNaN(endDate.getTime())
) {
// this warning is not valid, dont try to call getLocalDateFromEST
Owner

Do we really need this comment here? I personally think the comment on line 25, along with the general structure of the code, are already sufficient to convey what is happening and why.

Do we really need this comment here? I personally think the comment on line 25, along with the general structure of the code, are already sufficient to convey what is happening and why.
snedadah marked this conversation as resolved
a258wang reviewed 6 months ago
// check if current warning is in range
return (
startDate.getTime() < today.getTime() &&
endDate.getTime() > today.getTime()
Owner

Nitpick: would it make more sense to use <= and >= instead of < and >?

As well, is the comment on line 35 really necessary? I personally think the code is already descriptive enough on its own.

Nitpick: would it make more sense to use `<=` and `>=` instead of `<` and `>`? As well, is the comment on line 35 really necessary? I personally think the code is already descriptive enough on its own.
Poster
Collaborator

You are correct, it does make more sense to use >=, I'll update this, however getTime() returns a time in milliseconds, so the difference between > and >= will only be evident for one millisecond :)

You are correct, it does make more sense to use >=, I'll update this, however getTime() returns a time in milliseconds, so the difference between `>` and `>=` will only be evident for one millisecond :)
snedadah marked this conversation as resolved
a258wang reviewed 6 months ago
});
if (currentWarnings.length > 1) {
// we should not throw here, since this could happen a date far after the push was made, and no one would know they site is down unless someone visits
Owner

This comment confused me the first time I read it. Maybe we could consider wording it more succinctly, eg. // log an error instead of throwing, so the site can still be built.

This comment confused me the first time I read it. Maybe we could consider wording it more succinctly, eg. `// log an error instead of throwing, so the site can still be built`.
Poster
Collaborator

This is actually won't effect the site building, since this code is all client side.

This code will get run everytime someone visits the site. If we threw an error, the following could happen:

After a push to the repositry, the site will build fine and look completly fine when deployed. However, if on someday in the future, there is two warnings scheduled on that same day, on that day (and only on that day, not earlier), the site will stop working. (when someone visits the site, it would not load), and we would have no idea unless we actually visit the site.

I'll make sure to make the comment a bit more clear.

This is actually won't effect the site building, since this code is all client side. This code will get run everytime someone visits the site. If we threw an error, the following could happen: After a push to the repositry, the site will build fine and look completly fine when deployed. However, if on someday in the future, there is two warnings scheduled on that same day, on that day (and only on that day, not earlier), the site will stop working. (when someone visits the site, it would not load), and we would have no idea unless we actually visit the site. I'll make sure to make the comment a bit more clear.
snedadah marked this conversation as resolved
a258wang reviewed 6 months ago
);
}
return currentWarnings.length == 0 ? null : currentWarnings[0];
Owner

Is there a reason why we're using == instead of ===?

Is there a reason why we're using `==` instead of `===`?
snedadah marked this conversation as resolved
a258wang reviewed 6 months ago
export function WarningHeader() {
const warning = getCurrentWarning();
return warning == null ? null : (
Owner

Similar to above - is there a reason why we're using == instead of ===?

Also, a nitpick: personally it looks a little weird to me that we have ... == null ? null : ... - the fact that we have null show up twice makes my brain go 🤔, even though this works just fine. Maybe we could consider using

return warning ? <div>...</div> : null;

or

return (warning && <div>...</div>);

if either of these options seem more readable/less awkward?

Similar to above - is there a reason why we're using `==` instead of `===`? Also, a nitpick: personally it looks a little weird to me that we have `... == null ? null : ...` - the fact that we have `null` show up twice makes my brain go 🤔, even though this works just fine. Maybe we could consider using ```react return warning ? <div>...</div> : null; ``` or ```react return (warning && <div>...</div>); ``` if either of these options seem more readable/less awkward?
Poster
Collaborator

The reason behind doing it this way was:
#394
and #394

Also, if you use ===, then undefined === null is false, but undefined == null is true, so the second way is prefered when doing null checks.

The reason behind doing it this way was: https://git.csclub.uwaterloo.ca/www/www-new/pulls/394#issuecomment-7393 and https://git.csclub.uwaterloo.ca/www/www-new/pulls/394#issuecomment-7390 Also, if you use `===`, then `undefined === null` is false, but `undefined == null` is true, so the second way is prefered when doing null checks.
Owner

Ahh okay, good point about the null checks!

As for the other point, the way I see it, warning is always going to be either a Warning object or null, so in this situation checking if warning is null is equivalent to checking if it is falsy. Both of the options I suggested will return null if warning is null. That being said, this is honestly just a matter of personal preference, so I'll let you decide what you prefer (unless someone else wants to weigh in with their opinion).

Ahh okay, good point about the null checks! As for the other point, the way I see it, `warning` is always going to be either a `Warning` object or `null`, so in this situation checking if `warning` is null is equivalent to checking if it is falsy. Both of the options I suggested will return `null` if `warning` is `null`. That being said, this is honestly just a matter of personal preference, so I'll let you decide what you prefer (unless someone else wants to weigh in with their opinion).
snedadah added 1 commit 6 months ago
936d7547f9 updated comments and did minor refactoring
a258wang approved these changes 6 months ago
a258wang left a comment
Owner

Looks good to me! Thank you Shahan for your thoughtfulness and your responsiveness to feedback.

Looks good to me! Thank you Shahan for your thoughtfulness and your responsiveness to feedback.
snedadah added 1 commit 6 months ago
a0d25c9ee1 Merge branch 'main' into warning-header
snedadah merged commit bb78a3d53d into main 6 months ago
snedadah referenced this issue from a commit 6 months ago

Reviewers

n3parikh was requested for review 7 months ago
j285he was requested for review 7 months ago
a258wang approved these changes 6 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as bb78a3d53d.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.