Fixes #1576, Added FRONTEND env variable, updated routes/index.js #1606

Metropass · 2021-01-25T21:42:59Z

Issue This PR Addresses

Closes #1576
Created a FRONTEND variable for all of the .env files, and added a conditional if within backend/routes/index.js
Currently, the only two values that can be accepted for FRONTEND are: gatsby and next
gatsby is the default value as of this Pull Request

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

humphd · 2021-01-25T22:08:40Z

env.example

+#   'gatsby' - The Gatsby Front-End
+#   'next' - The NextJs Front-End
+# Our default value for FRONTEND is 'gatsby'
+FRONTEND='gatsby'


Remove the surrounding quotes, all values are strings:

FRONTEND=gatsby

Will do, thanks!

humphd · 2021-01-25T22:09:14Z

netlify.toml

@@ -8,3 +8,4 @@
  API_URL="https://dev.telescope.cdot.systems"
  # We need to expose the API_URL to the front-end too
  NEXT_PUBLIC_API_URL="https://dev.telescope.cdot.systems"
+  FRONTEND='gatsby'


Should be next on Netlify.

humphd · 2021-01-25T22:10:27Z

src/backend/web/routes/index.js

@@ -17,7 +17,7 @@ const user = require('./user');
 const query = require('./query');

 const router = express.Router();
-
+const { FRONTEND } = process.env.FRONTEND;


In the rest of this code, we seem to be doing process.env.FRONTEND inline, instead of pulling things out. Can you match that below?

Careful here: using destructuring, you have to remove the name of the variable in the rhs.

const { FRONTEND } = process.env;

Will do! Everything should be fixed along with the rest of your suggestions

humphd · 2021-01-25T22:12:14Z

src/backend/web/routes/index.js

+  } else if (FRONTEND === 'next') {
+    router.use(express.static(path.join(__dirname, '../../../frontend/next/out')));
+  } else {
+    throw new Error(


Let's test that this does what we want (e.g., we should probably crash right now).

humphd · 2021-01-25T22:12:54Z

src/backend/web/routes/index.js

    // Or serve the static files in the Gatsby build directory
+  } else if (FRONTEND === 'gatsby') {


Let's make sure that case doesn't affect these (e.g., .toLowerCase() on the frontend values we compare against).

humphd

Looks good. @Metropass, please update the team tomorrow about this change so people can update their local .env, since it will throw right now if it's missing.

Question related to that: should we default to Gatsby if it's missing instead of throwing? That is:

gatsby: use gatsby
next: use next
{not set}: use gatsby
anything else: throw

humphd · 2021-01-26T01:31:37Z

src/backend/web/routes/index.js

@@ -17,7 +17,6 @@ const user = require('./user');
 const query = require('./query');

 const router = express.Router();
-


Watch out for spurious whitespace changes like this, as it churns git history unnecessarily.

HyperTHD · 2021-01-26T02:29:18Z

Looks good. @Metropass, please update the team tomorrow about this change so people can update their local .env, since it will throw right now if it's missing.

Question related to that: should we default to Gatsby if it's missing instead of throwing? That is:

gatsby: use gatsby

next: use next

{not set}: use gatsby

anything else: throw

I say for now, we should have it default to gatsby since the next transition is not complete yet. Whenever that's complete, unless we have any sort of reason to keep gatsby, we can just have it serve next, and throw if anything else other than that is used

humphd · 2021-01-29T17:56:11Z

src/backend/web/routes/index.js

@@ -45,13 +45,13 @@ if (process.env.NODE_ENV === 'development') {
    // Allow proxying the Gatsby dev server through our backend if PROXY_FRONTEND=1 is set in env
    router.use('/', createProxyMiddleware({ target: 'http://localhost:8000', changeOrigin: true }));
    // Or serve the static files in the Gatsby build directory
-  } else if (process.env.FRONTEND.toLowerCase() === 'gatsby') {
+  } else if (process.env.FRONTEND.toLowerCase() === 'gatsby' || process.env.FRONTEND === '') {


Question: will process.env.FRONTEND.toLowerCase() break if FRONTEND isn't defined? That is, should this be:

else if(!process.env.FRONTEND || process.env.FRONTEND === '' || process.env.FRONTEND.toLowerCase() === 'gatsby') {...}

Oh, that's a good point, I'll make that change real quick

…o env-frontend

manekenpix · 2021-02-07T21:24:05Z

is this ready for review?

Metropass · 2021-02-08T18:00:25Z

@manekenpix Should be ready now!

chrispinkney · 2021-02-10T19:59:56Z

@Metropass This might be a stupid question but how can I test this? Also does this mean I have to copy a new example .env file after this merges?

chrispinkney · 2021-02-10T20:02:36Z

env.example

@@ -4,6 +4,16 @@ NODE_ENV=development
 # PORT is the port used by the web server
 PORT=3000


Does this change affect this line?

It does, thanks for bring it up.
It seems like I need to make a conditional statement within the package.json
@humphd cc'ing just for clarity, Are we allowed to make conditionals in our packages for Telescope?

I don't think they're related. PORT=3000 is for telescope (telescope serving the back end), The change @chrispinkney is talking about (this) is for next's production server (a separate server to serve the production build, but telescope has nothing to do with it).

Metropass · 2021-02-10T20:12:37Z

@Metropass This might be a stupid question but how can I test this? Also does this mean I have to copy a new example .env file after this merges?

It's not a stupid question at all!
Just

@Metropass This might be a stupid question but how can I test this? Also does this mean I have to copy a new example .env file after this merges?

It's not a stupid question at all!
Just have the FRONTEND variable set to which ever Front-End service you'd like to run eg, FRONTEND=gatsby, and index.js should take care of the rest.

chrispinkney

Otherwise LGTM!

Start redis and elasticsearch in docker:
npm run build
npm run build:next
set FRONTEND=gatsby
npm start
set FRONTEND=next
npm start

Hope this is right 😄. Everything started up and ran

chrispinkney · 2021-02-12T17:23:59Z

env.example

@@ -4,6 +4,16 @@ NODE_ENV=development
 # PORT is the port used by the web server
 PORT=3000



Haha sorry, I know it's pedantic, but can you get rid of the extra newlines on line 6 and 15?

HyperTHD

This PR looks okay. We're switching to next and not gatsby so we can make this exclusively next instead. You'll need to rebase and fix conflicts here too.

Read #1733 for context

chrispinkney · 2021-02-14T16:16:11Z

This PR looks okay. We're switching to next and not gatsby so we can make this exclusively next instead. You'll need to rebase and fix conflicts here too.

Read #1733 for context

Hm good point. In fact AFAIK we'll be removing Gatsby entirely when Next finally lands. Not sure! Something to think about I guess @Metropass

humphd · 2021-02-14T17:33:03Z

This was started nearly a month ago, and we needed it then, but at this point, I don't think it's useful, since we're moving to a single front-end. Closing.

Added FRONTEND env variable, updated routes/index.js

2b7174d

vercel bot deployed to Preview January 25, 2021 21:43 View deployment

Added FRONTEND assignments within the netlify/vercel config

78de7d7

Metropass requested review from humphd, manekenpix and HyperTHD January 25, 2021 21:44

humphd requested changes Jan 25, 2021

View reviewed changes

Metropass added 2 commits January 25, 2021 17:25

Removed Quotes

85ed782

Added toLowerCase, fixed netlify path from gatsby to next

41f4898

manekenpix assigned Metropass Jan 26, 2021

manekenpix added the area: back-end label Jan 26, 2021

manekenpix requested a review from humphd January 26, 2021 01:18

vercel bot deployed to Preview January 26, 2021 01:30 View deployment

humphd requested changes Jan 26, 2021

View reviewed changes

birtony mentioned this pull request Jan 26, 2021

Update the default FRONTEND env variable value to Next #1609

Closed

Added variable echo, replaced quotes with backticks to satisfy eslint

6bcba64

humphd reviewed Jan 29, 2021

View reviewed changes

Metropass added 2 commits February 3, 2021 13:15

Merge branch 'master' of https://github.com/Seneca-CDOT/telescope int…

0f448e9

…o env-frontend

Check if FRONTEND variable isn't defined

525955c

yuanLeeMidori requested a review from humphd February 9, 2021 15:10

chrispinkney reviewed Feb 10, 2021

View reviewed changes

manekenpix requested a review from chrispinkney February 11, 2021 20:02

chrispinkney approved these changes Feb 12, 2021

View reviewed changes

HyperTHD requested changes Feb 13, 2021

View reviewed changes

humphd closed this Feb 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1576, Added FRONTEND env variable, updated routes/index.js #1606

Fixes #1576, Added FRONTEND env variable, updated routes/index.js #1606

Metropass commented Jan 25, 2021 •

edited by manekenpix

Loading

humphd Jan 25, 2021

Metropass Jan 25, 2021

humphd Jan 25, 2021

humphd Jan 25, 2021

manekenpix Jan 25, 2021

Metropass Jan 25, 2021

humphd Jan 25, 2021

humphd Jan 25, 2021

humphd left a comment

humphd Jan 26, 2021

HyperTHD commented Jan 26, 2021 •

edited

Loading

humphd Jan 29, 2021

Metropass Feb 3, 2021

manekenpix commented Feb 7, 2021

Metropass commented Feb 8, 2021

chrispinkney commented Feb 10, 2021

chrispinkney Feb 10, 2021 •

edited

Loading

Metropass Feb 10, 2021

manekenpix Feb 10, 2021

Metropass commented Feb 10, 2021

chrispinkney left a comment •

edited

Loading

chrispinkney Feb 12, 2021

HyperTHD left a comment •

edited

Loading

chrispinkney commented Feb 14, 2021

humphd commented Feb 14, 2021

		// Or serve the static files in the Gatsby build directory
		} else if (FRONTEND === 'gatsby') {

		@@ -17,7 +17,6 @@ const user = require('./user');
		const query = require('./query');

		const router = express.Router();

		@@ -4,6 +4,16 @@ NODE_ENV=development
		# PORT is the port used by the web server
		PORT=3000

Fixes #1576, Added FRONTEND env variable, updated routes/index.js #1606

Fixes #1576, Added FRONTEND env variable, updated routes/index.js #1606

Conversation

Metropass commented Jan 25, 2021 • edited by manekenpix Loading

Issue This PR Addresses

Type of Change

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyperTHD commented Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manekenpix commented Feb 7, 2021

Metropass commented Feb 8, 2021

chrispinkney commented Feb 10, 2021

chrispinkney Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Metropass commented Feb 10, 2021

chrispinkney left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyperTHD left a comment • edited Loading

Choose a reason for hiding this comment

chrispinkney commented Feb 14, 2021

humphd commented Feb 14, 2021

Metropass commented Jan 25, 2021 •

edited by manekenpix

Loading

HyperTHD commented Jan 26, 2021 •

edited

Loading

chrispinkney Feb 10, 2021 •

edited

Loading

chrispinkney left a comment •

edited

Loading

HyperTHD left a comment •

edited

Loading