Skip to content
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

Merge next into master, include both Gatsby.js and Next.js #1444

Merged
merged 15 commits into from
Nov 25, 2020

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Nov 23, 2020

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

Description

I want to make it easier to run the Gatsby and Next frontends side-by-side. The current branch-based separation makes it really hard to switch back and forth with dependencies. To do that, I've merged the next branch into master, and created a temporary parallel front-end structure:

└──frontend
      └──next        
      └──gatsby

Our default front-end continues to be Gatsby, and all references to src/frontend have been updated to src/frontend/gatsby.

I've adjusted our scripts to work for both next and gatsby, and added temporary docs in src/frontend/README.md.

I've renamed the env variable PROXY_GATSBY To PROXY_FRONTEND, since it will work for both cases.

I've cleaned up the next.js deps and eslint config to be more accurate and up-to-date.

I've separated the front-end specific bits of .gitignore into each of the frontend dirs.

I think everything is working, but check my math please.

@humphd
Copy link
Contributor Author

humphd commented Nov 23, 2020

This is ready for review. I've streamlined the eslint stuff so we only have a single code base, but it works across node, JS, and TS. I had to fix a bunch of existing lint issues that were hiding in our code.

c3ho
c3ho previously approved these changes Nov 23, 2020
Copy link
Contributor

@c3ho c3ho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with npm run develop was able to run it with local backend. All expected front-end functionality was still working. Also ran npm run develop:next and it built successfully

@humphd
Copy link
Contributor Author

humphd commented Nov 23, 2020

For testing this, I'll up-level the docs that are buried in this PR:


Telescope Frontend Development

Introduction

Telescope is in the process of porting the front-end from GatsbyJS
to Next.js, see #1316.

To facilitate this conversion, we are hosting both front-ends in the src/frontend folder. For
the time being, the root npm scripts will default to run the GatsbyJS front-end. However, when
we finish the conversion, we'll switch that to Next.js and delete the GatsbyJS code.

NOTE: if you are only working on the front-end code, set PROXY_FRONTEND=1 in your .env to use
the development server (https://dev.telescope.cdot.systems) as your back-end.

Running GatsbyJS

From the root of the project, you can run a number of GatsbyJS specific npm scripts:

  1. npm run develop to start the development server
  2. npm run build to build the site in src/frontend/gatsby/public for production
  3. npm run serve to serve the production site in src/frontend/gatsby/public for testing
  4. npm run clean to delete generated build files and folders

Running Next.js

From the root of the project, you can run a number of GatsbyJS specific npm scripts:

  1. npm run develop:next to start the development server
  2. npm run build:next to build the site in src/frontend/next/public for production
  3. npm run serve:next to serve the production site in src/frontend/next/.next for testing

Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In gatsby-config.js, line 7, it needs an extra ../:

path: path.join(__dirname, '../../../', '.env'),

@humphd
Copy link
Contributor Author

humphd commented Nov 25, 2020

@manekenpix great catch, thanks for reviewing this so closely. I'll correct and rebase soon to deal with these new changes.

@humphd
Copy link
Contributor Author

humphd commented Nov 25, 2020

Pushed a fix for the gatsby-config.js issue. I think I'll wait for approval to do a rebase at the end, since it's going to be a bit of work.

@humphd humphd requested a review from manekenpix November 25, 2020 01:40
@chrispinkney
Copy link
Contributor

NOTE: if you are only working on the front-end code, set PROXY_FRONTEND=1 in your .env to use
the development server (https://dev.telescope.cdot.systems) as your back-end.

Also, API_URL should be set to API_URL=https://dev.telescope.cdot.systems as I was getting no timeline until this was set. Might be an obvious thing that I just missed though, haha.

@chrispinkney
Copy link
Contributor

Tested all the above commands using your remote, runs both Gatsby and Next start page; LGTM. (always wanted to say that)

Copy link
Contributor

@cindyorangis cindyorangis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this using Docker on Windows and I'm able to run both Gatsby and Next frontends :D

@humphd humphd requested a review from c3ho November 25, 2020 16:56
@humphd
Copy link
Contributor Author

humphd commented Nov 25, 2020

@manekenpix
Copy link
Member

@humphd Vercel failed, and there are still conflicts. Are we ok merging with the conflicts?

@humphd
Copy link
Contributor Author

humphd commented Nov 25, 2020

Vercel is just wanting me to approve the changes to the config (happens every time I push), and I'll fix merge conflicts once it's approved.

@humphd humphd merged commit 6195af3 into Seneca-CDOT:master Nov 25, 2020
@humphd
Copy link
Contributor Author

humphd commented Nov 25, 2020

I've now merged this on master (see 6195af3) and it's quite possible I've broken things. We'll see how it does on https://dev.telescope.cdot.systems in a few mins, and adjust as needed.

I the mean time, I wanted to mention a few things to everyone that has open PRs touching the frontend (@manekenpix, @lixiaoqity, @phast184, @NathanPang001, @abhaseen, @PedroFonsecaDEV, @jiyoungsin, @c3ho, @tianlangwu):

  1. you should update your master branch git checkout master and git pull upstream master to get this change.
  2. you should run npm install again
  3. you should rebase your branch on master git checkout your-branch and git rebase master. Fix any conflicts.
  4. when you finish the rebase, confirm that you don't have new code in src/frontend/* and that all your changes are in src/frontend/gatsby/* (i.e., all frontend code has been moved to gatsby/).

If you have problems or need help, let us know in your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants