-
Notifications
You must be signed in to change notification settings - Fork 0
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
Show all episodes in Netlify deploy previews #91
base: master
Are you sure you want to change the base?
Conversation
✔️ Deploy Preview for koodikrapula ready! 🔨 Explore the source changes: a3b1470 🔍 Inspect the deploy log: https://app.netlify.com/sites/koodikrapula/deploys/617805d09d426a000775a952 😎 Browse the preview: https://deploy-preview-91--koodikrapula.netlify.app |
.filter((episode) => !(isProdBuild() && isScheduled(episode))) | ||
.filter( | ||
(episode) => | ||
!(isProdBuild() && isScheduled(episode)) || isPreviewBuild() |
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.
Rhetorical question: what does this do?
I think it's not obvious at a glance, so I created a truth table (1 = true, 0 = false):
is prod build | is scheduled | is deploy preview | is prod build && is scheduled | !(is prod build && is scheduled) | !(is prod build && is scheduled) || is deploy preview | |
---|---|---|---|---|---|---|
1 | 1 | 1 | 1 | 0 | 1 | |
1 | 1 | 0 | 1 | 0 | 0 | |
1 | 0 | 1 | 0 | 1 | 1 | |
0 | 1 | 1 | 0 | 1 | 1 | |
1 | 0 | 0 | 0 | 1 | 1 | |
0 | 1 | 0 | 0 | 1 | 1 | |
0 | 0 | 1 | 0 | 1 | 1 | |
0 | 0 | 0 | 0 | 1 | 1 |
And here's a simpler truth table with descriptions in English (notice how the rightmost column is identical to the previous table's rightmost column except here I'm using the words "yes" and "no" instead of 1 and 0):
is prod build | is scheduled | is deploy preview | should this episode be included? |
---|---|---|---|
1 | 1 | 1 | yes because is deploy preview |
1 | 1 | 0 | no |
1 | 0 | 1 | yes because is not scheduled / is deploy preview |
0 | 1 | 1 | yes because is dev build / deploy preview (invalid combo because deploy previews are always prod builds) |
1 | 0 | 0 | yes because is not scheduled |
0 | 1 | 0 | yes because is dev build |
0 | 0 | 1 | yes because is dev build / not scheduled / is deploy preview (invalid combo; see above) |
0 | 0 | 0 | yes because is dev build / not scheduled |
All right, so .filter()
should return false
only when isProdBuild() && isScheduled() && !isNetlifyDeployPreview()
. Like so:
.filter(
(episode) => !(isProdBuild() && isScheduled(episode) && !isNetlifyDeployPreview())
)
Would that be easier to grasp? Or maybe move isScheduled()
to the beginning so it's clearer that this filtering is about scheduled episodes:
.filter(
(episode) => !(isScheduled(episode) && isProdBuild() && !isNetlifyDeployPreview())
)
Using one of De Morgan's laws – !(A && B) === !A || !B
– the conditional could be flipped on its head:
.filter(
(episode) => !isScheduled(episode) || !isProdBuild() || isNetlifyDeployPreview()
)
The conditional could be even easier to read if we created a new util isDevBuild()
:
.filter(
(episode) => !isScheduled(episode) || isDevBuild() || isNetlifyDeployPreview()
)
Hmm, I take my words back – I think the conditional is clearer if isScheduled()
is at the end:
.filter(
(episode) => isDevBuild() || isNetlifyDeployPreview() || !isScheduled(episode)
)
Now it reads like English: "include all episodes in dev builds and Netlify deploy previews; otherwise include non-scheduled episodes." I think this would be the best option!
Which one is the best in your opinion?
/** | ||
* Checks whether the current build is a deploy preview build. | ||
* | ||
* @returns {boolean} | ||
*/ | ||
export const isPreviewBuild = () => process.env.NODE_ENV === 'deploy-preview' |
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.
- We already have a util called
isNetlifyProdEnv()
, soisNetlifyDeployPreview()
would have a more consistent name. - The check should be against
process.env.CONTEXT
, notprocess.env.NODE_ENV
. - With a similar JSDoc comment as in
isNetlifyProdEnv()
, the code could look like this:
/** | |
* Checks whether the current build is a deploy preview build. | |
* | |
* @returns {boolean} | |
*/ | |
export const isPreviewBuild = () => process.env.NODE_ENV === 'deploy-preview' | |
/** | |
* Checks whether the current build is a Netlify deploy preview. | |
* Netlify does automatic deploy previews for pull requests. | |
* | |
* Returns `false` in non-Netlify environments, e.g. localhost. | |
* | |
* @see {@link https://docs.netlify.com/configure-builds/environment-variables/#build-metadata} | |
* | |
* @returns {boolean} | |
*/ | |
export const isNetlifyDeployPreview = () => process.env.CONTEXT === 'deploy-preview' |
I would also:
- Move this util to the beginning of the file so that all utils are sorted alphabetically.
- Simplify the JSDoc comment of
isNetlifyProdEnv()
:
/**
* Checks whether the current build is against Netlify's production.
*
* Returns `false` in non-Netlify environments, e.g. localhost.
*
* @see {@link https://docs.netlify.com/configure-builds/environment-variables/#build-metadata}
*
* @returns {boolean}
*/
export const isNetlifyProdEnv = () => process.env.CONTEXT === 'production'
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.
Can't be merged yet because at least this change is necessary:
-process.env.NODE_ENV === 'deploy-preview'
+process.env.CONTEXT === 'deploy-preview'
Fixes #79.