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

Refactor project types & add Next.js #292

Merged
merged 13 commits into from
Nov 1, 2018
Merged

Conversation

AWolf81
Copy link
Collaborator

@AWolf81 AWolf81 commented Oct 14, 2018

Related Issue:
#59

Summary:
This branch is based on branch nextjs-flow (flow for workflow). I first started with this and I pushed it just for completeness. After merging the refactor branch we can safely delete the other branch.
I've merged the process-logging branch into this branch so it was easier to develop the refactoring.

  • Created a configuration file inside src/config/project-types.js which contains three objects for storing the following information:
    • devServer task name
    • devServer command args & port handling with "template" variable
  • Modified isDevServerTask, getDevServerTaskForProjectId & getTaskType to use the configuration object.

As mentioned in the issue comment I first had the idea of having a function inside of the configuration object but I don't like that.
Now I'm using the string $port inside of the configuration that will be replaced with the real port number in code. Just top-level replacement of $port is not working but I think we could add this later. Also not sure if we need to support deeply nesting as it is only working for one level e.g.

{
  env: {
    PORT: '$port'
  }
}

Will be replaced to (if available port is 3000):

{
  env: {
    PORT: 3000
  }
}

I think this is also the first step for improving the Gatsby workflow. Do we have an issue for Gatsby improvement? I think I read that on Gitter but I'm not sure what should be improved.

Things to improve / discuss:

  • Nextjs logo is a bit small and I don't like that we're having Next.js text twice. Maybe we could remove the second text. (see screeshot below)
  • Is the approach with the configuration in one file OK? I think that's better than the dynamic require. I'm also not sure if configuration is the right word for it. It's more like a mapping but I don't have a better word.
  • I tried to create a common structure with the configuration object but there is at least one position that is still adding some noise. e.g. task.saga line 179 adds additionalArgs to the command if test task running. We could also add this to the configuration but I think we can modify this later.
  • Should we swallow some log messages from Next.js? I think we don't need that many messages.

Todo

  • Check Gatsby Format & Test on master. It fails on this branch. This issue is not related to this branch. Same behaviour on master. Needs to be tackled separately.
  • I need to file an issue at create-next-app because with-out my fork it wasn't working with the projectDirectory passed to the script npx create-next-app c:\users\alexander\Guppy-dev\test wasn't working. Maybe I can create a PR there if no other solution is available. OK, added a comment here
  • Check Package.json of Next.js project and why there is start task missing. There is a start task but it's not visible in Guppy as Melanie mentioned. Checked start script Seems like it requires to run build before. So dev is the right command to start the development server. Start is not displayed as we're only filtering by task name so it gets removed from tasks list. I'll update this so we're more specific e.g. start & create-react-app. OK, I've modified this so the filtering is working but there is another issue because start from next.js is also a long-running script which launches a local server we're having trouble with it. I'll open an issue for it so I can describe it in detail.
  • Add triangle next icon as Aaron proposed
  • Modify comment for isDevServer as proposed by j-f1

Screenshots/GIFs:
Project create with Next.js
grafik

Next.js project
grafik
(failed build because I tested task stopping.)

@codecov
Copy link

codecov bot commented Oct 14, 2018

Codecov Report

Merging #292 into master will increase coverage by 0.44%.
The diff coverage is 51.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
+ Coverage   20.09%   20.54%   +0.44%     
==========================================
  Files         235      237       +2     
  Lines        3648     3690      +42     
  Branches      368      369       +1     
==========================================
+ Hits          733      758      +25     
- Misses       2648     2667      +19     
+ Partials      267      265       -2
Impacted Files Coverage Δ
src/config/app.js 100% <ø> (ø) ⬆️
src/components/CreateNewProjectWizard/MainPane.js 0% <0%> (ø) ⬆️
src/services/dependencies.service.js 25% <0%> (-1.32%) ⬇️
...rc/components/TaskDetailsModal/TaskDetailsModal.js 0% <0%> (ø) ⬆️
...nts/DevelopmentServerPane/DevelopmentServerPane.js 0% <0%> (ø) ⬆️
src/services/project-type-specifics.js 83.33% <0%> (-16.67%) ⬇️
src/components/TaskRunnerPane/TaskRunnerPane.js 0% <0%> (ø) ⬆️
src/services/process-logger.service.js 33.33% <33.33%> (ø)
src/actions/index.js 93.39% <50%> (-0.89%) ⬇️
src/reducers/tasks.reducer.js 61.46% <58.82%> (+1.46%) ⬆️
... and 5 more

Copy link
Collaborator

@melanieseltzer melanieseltzer left a comment

Choose a reason for hiding this comment

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

This is awesome! I definitely think adding support for Next is valuable. I have some notes and suggestions, and also a question:

create-next-app has a start task in package.json (running next start) but it's not getting picked up to display in Guppy. Is it because the word 'start' is flagged as a dev server task name (for CRA)?

Also, responses to your questions:

Nextjs logo is a bit small and I don't like that we're having Next.js text twice. Maybe we could remove the second text.

Personally I don't mind it and actually quite like the way it looks, but I can see how it could bother people. It might look weird to just have the logo without text, if the other two follow a different look?

Is the approach with the configuration in one file OK?

I like it better than dynamic requires 👍 Interested to hear other people's thoughts on this though.

Should we swallow some log messages from Next.js? I think we don't need that many messages.

I think that might be a good idea if it's an easy thing to do, but I don't think it's critical. Although for the dev task in particular, it would be cool to replicate the progress bar - I wonder if there's a way to do that?

@@ -251,7 +256,7 @@ export const getTaskDescription = (name: string) => {

export const isDevServerTask = (name: string) =>
// Gatsby and create-react-app use different names for the same task.
name === 'start' || name === 'develop';
devServerTaskNames.indexOf(name) !== -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: add next.js to comment

// Gatsby, create-react-app and next.js use different names for the same task.

Also any reason not to use includes() here rather than indexOf()? I find it a lot more intuitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you're right. includes is better as it's more intuitive. I'll change the comment & the includes later today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps “Each framework uses a different name for the task that starts the development server”? That way, it doesn’t have to be changed for each new framework.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@j-f1 Even better :)

src/sagas/task.saga.js Show resolved Hide resolved
<Spacer inline size={10} />
<ButtonWithIcon
showStroke={projectType === 'nextjs'}
icon={<GatsbyIcon src={nextjsIconSrc} />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think since GatsbyIcon is getting re-used now on something non Gatsby, we should generalize it (and ReactIcon) and have width and height props:

<IconImage width={'...'} height={'...'} src={...} />
const IconImage = styled.img`
  width: ${props => props.width || '32px'};
  height: ${props => props.height || '32px'};
`;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good catch. That was my mistake I should have created a NextjsIcon - I missed that point.
I think I'll do the mapping as mentioned in line 83. So we only have the IconImage and map over the configuration keys to render the project type selection icons.

@@ -79,6 +80,7 @@ class MainPane extends PureComponent<Props> {
isFocused={activeField === 'projectType'}
>
<ProjectTypeTogglesWrapper>
{/* Todo: Make it easier to add new flows - e.g. map over an array to generate the UI*/}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@superhawk610
Copy link
Collaborator

Looking around, I've seen this Next.js logo used a couple places, I think it would fit slightly better with the existing icons?

next

@AWolf81 AWolf81 changed the title [WIP] Refactor project types & add Next.js Refactor project types & add Next.js Oct 20, 2018
@AWolf81
Copy link
Collaborator Author

AWolf81 commented Oct 31, 2018

@melanieseltzer I think I've addressed your review changes. Could you please have a look?

The only open things are:

  • Using my fork for create-next-app. I think we can change it later to the base repo once we're having a feedback from the maintainer (11 days with-out feedback).
  • The second dev server start script is a problem as it's not detected as long running (see separate issue)
  • Script description needs to be per project type (see separate issue)

I think we can tackle the open stuff afterwards.

Copy link
Collaborator

@melanieseltzer melanieseltzer left a comment

Choose a reason for hiding this comment

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

LGTM, I like the new triangle icon! Approving with some small non-critical comments. I think it can be merged as is since you've created the separate issues.

);
}
}
// todo: add top level substiution - not used yet but maybe needed later e.g. { env: $port } won't be replaced.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: typo (substitution)

// not sure if we need that nesting but I think there could be more to configure
args: projectPath => [
// used for project creation previous getBuildInstructions
'github:awolf81/create-next-app', // later will be 'create-next-app' --> issue not filed yet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps update the comment for 'github:awolf81/create-next-app' to reference the issue URL, for tracking?

@AWolf81 AWolf81 merged commit e7d8057 into master Nov 1, 2018
@AWolf81 AWolf81 deleted the refactor-project-types branch November 1, 2018 13:48
@AWolf81 AWolf81 mentioned this pull request Nov 2, 2018
@AWolf81 AWolf81 mentioned this pull request Nov 25, 2018
18 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants