Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

[Proposal] next.js support #59

Closed
bennygenel opened this issue Jul 15, 2018 · 5 comments
Closed

[Proposal] next.js support #59

bennygenel opened this issue Jul 15, 2018 · 5 comments
Labels
upcoming feature New feature or request
Milestone

Comments

@bennygenel
Copy link
Collaborator

I think next.js can be really good addition to this program since it also supports creating projects with npx or yarn create with lots of example projects.

For someone just started using React, setting up SSR can be really confusing so next.js's out of the box support can be really helpful to get things started.

I already read about the possible addition on "How it works" part of the readme but thought it might benefit from getting its own issue to track down and discuss.

I would very much like to work on this but since I don't have a MacOS system I might not be able to test anything. Other than that if someone has any ideas on me how to contribute I am open for suggestions.

Thank you for the great project.

@joshwcomeau
Copy link
Owner

Yeah, I like the idea of supporting Next.js as well! Thanks for creating the issue.

It's a mid-level priority to me; not something I feel is critical, but not a long-term vision either. I'm a tiny bit wary of the added complexity cost for adding it, although I think it's also a good opportunity to clean up some of the less-than-stellar stuff with Gatsby (effectively I want to avoid sprinkling project-specific conditionals across the codebase, and find a cleaner way to differentiate between projects).

I would very much like to work on this but since I don't have a MacOS system I might not be able to test anything. Other than that if someone has any ideas on me how to contribute I am open for suggestions.

Are you on Linux or Windows? If you're on Linux, it actually seems to run fine (see discussion in #45, we'll have a release soon to officially support building for it). Windows is a bit of a larger challenge, but that's coming soon as well (discussion tracked in #27)

@joshwcomeau joshwcomeau added the upcoming feature New feature or request label Jul 16, 2018
@bennygenel
Copy link
Collaborator Author

Are you on Linux or Windows? If you're on Linux, it actually seems to run fine (see discussion in #45, we'll have a release soon to officially support building for it). Windows is a bit of a larger challenge, but that's coming soon as well (discussion tracked in #27)

@joshwcomeau I'm on a Windows machine but good thing that I'm working on Windows support. So hopefully soon it is not going to be a big problem =)

It's a mid-level priority to me; not something I feel is critical, but not a long-term vision either. I'm a tiny bit wary of the added complexity cost for adding it, although I think it's also a good opportunity to clean up some of the less-than-stellar stuff with Gatsby (effectively I want to avoid sprinkling project-specific conditionals across the codebase, and find a cleaner way to differentiate between projects).

I agree that this addition would push the project on a cleaner state. If we implement the structure you mention on #63 (comment) (formatCommandForPlatform ) it would be a good start I think.

After road-map for this gets clearer I'll gladly work on it.

@joshwcomeau
Copy link
Owner

I agree that this addition would push the project on a cleaner state. If we implement the structure you mention on #63 (comment) (formatCommandForPlatform) it would be a good start I think.

Yep! Agreed.

Excited to see support coming in the future :)

@AWolf81
Copy link
Collaborator

AWolf81 commented Oct 14, 2018

@joshwcomeau I'm working on this. I haven't pushed any thing yet. Next.js it's working.

I also think that we need to refactor the task execution as it feels difficult to add new things because of editing multiple locations and as mentioned sprinkling some conditionals here and there.

So to be more precise the following needs to be refactored (we also need to discuss these points as there are probably multiple ways to refactor):

  • Mapping for devserver task are named differently (see tasks.reducer isDevServerTask)
    I think we could create a configuration file for each project type - so we can add that mapping there e.g. {devServerTaskName: 'development'} for Gatsby.
    The following names after the equal sign are the devServer tasks:
    • Gatsby = development
    • CRA = start
    • Next.js = dev
  • Same map from before can be used for tasks.reducer getDevServerTaskForProjectId method. Then we can use that mapping Object instead of the switch block. I think something like return state.tasks[props.projectId][config.devServer.taskName] // config require with ${projectType}.js, see below
  • getDevServerCommand in task.saga needs to load the command args from the configuaration file so we can remove the switch and loadin it directly.
  • create-project.service function getBuildInstructions can also use the configuration object.
  • Inside create-project.service there is a Gatsby specific line like below. I think we can remove the conditional and always set it to sluggified projectName.
    if (projectType === 'gatsby') {
      packageJson.name = projectDirectoryName;
    }
  • For the devserver we also need to differentiate between sustained and short term (see getTaskType function in tasks.reducer). We can also add that info to the configuration object.

I think we could create a folder in src/project-type-configuration and store the configuration files there:

  • cra.js for CRA configuration
  • gatsby.js for Gatsby
  • nextjs.js for Next.js.

These file will return a configuration object e.g. for CRA:

export default {
  devServer: {
   taskName: 'start',
   args: ['run', 'start'],
   env: (port) => ({
     PORT: port
   }),
  create: {
     // not sure if we need that nesting but I think there could be more to configure
     args: (projectPath) => [
       // used for project creation previous getBuildInstructions
       'create-react-app', 
       projectPath 
     ]
  }
}

Usage for getDevServerCommand would be like following (untested code):

export const getDevServerCommand = (
  task: Task,
  projectType: ProjectType,
  port: string
) => {
  const config = require(`src/project-type-configuration/${projectType}`);
  return {
   args: config.args,
   env: config.env(port)
  };
}

And for isDevServerTask:

export const isDevServerTask = (name: string, projectType: ProjectType) => {
  const config = require(`src/project-type-configuration/${projectType}`);
  return name === config.devServer.taskName;
} 

Usage in getBuildInstructions in create-project.service:

export const getBuildInstructions = (
  projectType: ProjectType,
  projectPath: string
) => {
  // For Windows Support
  // Windows tries to run command as a script rather than on a cmd
  // To force it we add *.cmd to the commands
  const command = formatCommandForPlatform('npx');
  const config = require(`src/project-type-configuration/${projectType}`);
  return [
    command,
    ...config.create.args(projectPath)
  ];
}

I think this could work. I just don't like the dynamic requires that much but I think it's easier to add new project types. But it would be also OK to create one file with the configuration for each project and then we only need to import that file

Another point that's looking not that clean are the functions inside the configuration but that's probably OK. I'll check later if we could add some sort of config variables e.g. $projectPath that we'll replace later with the real variable - so we could remove the function.

Please let me know if that's a way to go or if we should do that differently.

Is it OK to refactor and add Next.js with one PR?
I think the Next.js part is really small as I'm using npx create-next-app with a small fix to an issue that I need to file at that repo - seems like path handling at create-next-app is not working as expected.

@AWolf81 AWolf81 added this to the v0.4.0 milestone Oct 28, 2018
@AWolf81
Copy link
Collaborator

AWolf81 commented Nov 2, 2018

We can close this as it's merged with PR #292 into master.
🎉 Yay, we're now supporting Next.js as well.

Just two new issues need to be addressed to improve it.
Second long-running task & default task descriptions per project type. See issues #297 & #296.

@AWolf81 AWolf81 closed this as completed Nov 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
upcoming feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants