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

Refactor folder structure #296

Merged
merged 2 commits into from
Jun 10, 2020

Conversation

groodt
Copy link
Contributor

@groodt groodt commented Jun 4, 2020

Commuter uses a combination of expressjs (for the server and api) and nextjs (for the pages).

Nextjs supports the following:

I propose a migration fully onto nextjs to replace expressjs.

I think this more opinionated framework will make the code simpler and remove most of the manual routing logic with nextjs conventions. It also has the additional benefit of potentially hitting one of the roadmap items of "Configurable base path for commuter app".

In preparation of this, I've restructured the folders into something that I find more pleasing to work with in JS projects and I think it will allow for easier refactoring from express to nextjs.

I'd love your thoughts @rgbkrk @captainsafia

@groodt groodt marked this pull request as ready for review June 4, 2020 22:59
@captainsafia
Copy link
Member

I'm fine with migrating fully to Next.JS. We use it in quite a few other nteract projects like our website, the nteract web app, and nteract play so it's not out of the question.

I'd be interested in figuring out what the delta between this and #292 is. It'll help us figure out if we should pursue the migration to TypeScript independently.

@rgbkrk
Copy link
Member

rgbkrk commented Jun 7, 2020

Wow, I've been hoping to see this approach. I'm personally in favor of this as the way forward. I'd rather use #292 as a reference for pieces we tackle next.

@groodt
Copy link
Contributor Author

groodt commented Jun 8, 2020

I wasn't aware of that PR. It would be great if the project can get a fresh sync from Netflix and migrate to Typescript. I agree, it makes far more sense to use that as a base before considering future work.

What is required to move that branch forward? Let me know if there is anywhere that I can assist @MSeal @rgbkrk @captainsafia

@captainsafia
Copy link
Member

@groodt We decided that #292 probably isn't worth merging. However, we can use the PR as a reference when migrating the codebase to TypeScript (e.g. copy type definitions and such where applicable to speed up the process).

With regards to the overall effort, I think the next steps are:

  1. Finish up and merge refactor of folder structure
  2. Migrate to Next.JS with Express-backend
  3. Move Express-based API endpoints to Next.JS's endpoint routing
  4. Migrate to TypeScript

I think we can probably move the migration to TypeScript to #2 but I figure it is better to tackle that once the codebase has settled a bit.

@groodt
Copy link
Contributor Author

groodt commented Jun 8, 2020

Ok, thanks @captainsafia Plan SGTM!

As far as item 1. Finish up and merge refactor of folder structure (this PR), it is done from my perspective. I've made the changes that I'd like to make (at least initially) and the functionality is all still working from my perspective and development process. I'd appreciate if somebody else gave things a spin as well. Maybe your process is different to mine.

Are there any other refactors people would like to see in this PR in terms of structure changes?

Otherwise, I believe this can be merged and migration to Next.JS API Routes can be looked at.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left one comment inline to address.

src/frontend.js Outdated Show resolved Hide resolved
@captainsafia
Copy link
Member

Merging now. Onward and upward!

@captainsafia captainsafia merged commit c03d5bb into nteract:master Jun 10, 2020
@MSeal
Copy link
Member

MSeal commented Jun 10, 2020

🥳

@rgbkrk
Copy link
Member

rgbkrk commented Jun 15, 2020

I'm going to go ahead and learn next.js API routes and see what else I can learn of next.js 9.4.

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.

4 participants