Refactor: Move @backstage/cli create-app to separate package #1745

ayshiff · 2020-07-26T15:22:16Z

Hey, I just made a Pull Request!

It creates another package create-app which contains the logic for downloading and executing templates.
I need some help to finish this refacto 👍

I have some questions about what I did:

Is there a place where I could put some utilities shared between different packages (the src/lib(paths-tasks...).ts utilities). As I need them inside create-app package, it makes it redundant.
I don't know if this is a good solution to require templates from another package ? I thought I understood from reading the issue that the package should not contain templates (Shouldn't the packages be independent from each other ?)
Should we move the e2e tests related to the create-app inside the package ?

✔️ Checklist

All tests are passing yarn test
Screenshots attached (for UI changes)
Relevant documentation updated
Prettier run on changed files
Tests added for new functionality
Regression tests added for bug fixes

Rugvip

Thanks! Sorry about the slow review 😅

Definitely agree we need a common place to put some of the modules in the CLI. Some of the stuff is needed in other places too, like the backend and config-loader. Completely fine with having the duplicated code for now and dealing with it in a followup.

Regarding the template the idea as to download it from git, but tbh I think we should just put it in a templates/ folder like in the CLI, and think about downloading it later. The main use-case for downloading is to be able to point to multiple different templates, and we don't have that 😁

I think the e2e tests can stay in the CLI, they're testing many different parts of the repo, but mostly related to the CLI.

packages/create-app/README.md

packages/create-app/package.json

Rugvip · 2020-07-28T15:29:47Z

packages/create-app/package.json

+    "commander": "^4.1.1",
+    "chalk": "^4.0.0",
+    "fs-extra": "^9.0.0",
+    "react-dev-utils": "^10.2.1",


I think we can get rid of this right? Afaik it's only used in the bundling parts of the cli.

Rugvip · 2020-07-28T15:36:34Z

packages/create-app/src/createApp.ts

@@ -107,7 +107,7 @@ export default async (cmd: Command): Promise<void> => {
  ];
  const answers: Answers = await inquirer.prompt(questions);

-  const templateDir = paths.resolveOwn('templates/default-app');
+  const templateDir = paths.resolveOwnRoot('packages/cli/templates/default-app');


So this won't be available when running via npx, as it assumes this monorepo layout we'll be executed as a package in node_modules. I'd say just move over the default-app template to this package instead. It does create a problem with the dependency on the template in app:diff, but tbh I think we can remove that for now as it isn't very useful in its current form anyway.

packages/create-app/src/index.ts

Rugvip · 2020-07-28T15:39:44Z

packages/create-app/src/lib/paths.ts

+  return resolvePath(ownDir, '../..');
+}
+
+export function findPaths(): Paths {


Planning to move this entire util and file to a separate package, so let's duplicate this for now and deal with it later.

Rugvip · 2020-07-28T15:40:31Z

packages/create-app/src/lib/tasks.ts

+  }
+}
+
+export async function templatingTask(


Will be sure to find a common place for these things too 😁

Rugvip

I think it's time to promote this to a proper PR 😎

Best double check that everything has been moved out of the app template in master btw. I think there were some additions recently.

ayshiff · 2020-07-31T09:11:32Z

I am a little bit struggling with making the e2e tests pass.

Rugvip · 2020-08-01T17:47:04Z

packages/create-app/package.json

+    "backstage"
+  ],
+  "license": "Apache-2.0",
+  "main": "src/index.ts",


Suggested change

"main": "src/index.ts",

"main": "dist/index.cjs.js",

I think e2e tests break because specifying src/index.ts as the entrypoint causes it to be included in the packages version, just like if you included it in the files list. That in turn causes the entrypoint script to believe that it's a local development package, i.e. isLocal is set to true.

Does that mean that I have to put the builder part into the package?

Nah I think this change should be enough

ayshiff#1 😁

Thanks @Rugvip 👍

create-app: packaging fixes

Rugvip

Thanks! Hoping that last addition did the trick 😁

stefanbuck · 2021-02-16T10:52:44Z

Hi,

I've just noticed that running yarn backstage-cli app:diff is failing with

Error: Failed to read template directory: ENOENT: no such file or directory, scandir '/Users/ramba/code/backstage/node_modules/@backstage/cli/templates/default-app'

Looks like changes made in this PR like moving files from packages/cli/templates/default-app/* to packages/create-app/templates/default-app/* is the root cause for this issue.

The fact that no one has noticed this (or at least there is not GitHub issue) makes me wonder if its worth keeping this functionallty at all. On the other hand I'm in the middle of upgrading backstage and without running npx @backstage/create-app I'm kind of lost. Any thoughts?

benjdlambert · 2021-02-16T11:25:32Z

@stefanbuck so you're right, I don't think people really use that way as much.

I'd recommend updating the backstage-cli dependency in your project, and then run yarn backstage-cli versions:bump. That will upgrade you to all the latest dependencies, and print change logs for you to go through with some breaking changes.

Feel free to contribute a fix to the this command too if you feel it's better :)

stefanbuck · 2021-02-16T12:16:49Z

Well, if no one was complaining about this command being broken for the past 6 months, I would rather tend to remove it. Would you would accept a PR for this?

benjdlambert · 2021-02-16T13:33:41Z

@Rugvip - thoughts?

Rugvip reviewed Jul 28, 2020

View reviewed changes

ayshiff added 3 commits July 30, 2020 01:09

refactor(cli): move create-app into his own package

a43d29e

refactor(create-app): remove useless extras

5eb1f2d

feat(create-app): requested changes

489a57b

ayshiff force-pushed the refactor/cli-create-app branch from e7a9f2a to 489a57b Compare July 29, 2020 23:10

ayshiff added 2 commits July 30, 2020 09:50

fix(cli): e2e tests createApp path

7bedd6f

fix(cli): add create-app import to e2e tests

730d1b2

Rugvip reviewed Jul 31, 2020

View reviewed changes

ayshiff marked this pull request as ready for review July 31, 2020 09:11

ayshiff requested a review from a team as a code owner July 31, 2020 09:11

Rugvip reviewed Aug 1, 2020

View reviewed changes

ayshiff and others added 4 commits August 2, 2020 13:46

fix(create-app): update entrypoint

5e78cc1

create-app: packaging fixes

d273c74

Merge pull request #1 from spotify/refactor/cli-create-app

21d8043

create-app: packaging fixes

feat(create-app): add missing template backend package

b5a6d82

Rugvip approved these changes Aug 2, 2020

View reviewed changes

Rugvip merged commit bd3d0de into backstage:master Aug 2, 2020

freben mentioned this pull request Aug 4, 2020

fix the wrong command #1814

Closed

stefanbuck mentioned this pull request Feb 18, 2021

chore: remove app:diff command from cli #4585

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Move @backstage/cli create-app to separate package #1745

Refactor: Move @backstage/cli create-app to separate package #1745

ayshiff commented Jul 26, 2020 •

edited

Rugvip left a comment

Rugvip Jul 28, 2020

Rugvip Jul 28, 2020

Rugvip Jul 28, 2020

Rugvip Jul 28, 2020 •

edited

Rugvip left a comment

ayshiff commented Jul 31, 2020

Rugvip Aug 1, 2020

ayshiff Aug 2, 2020

Rugvip Aug 2, 2020

Rugvip Aug 2, 2020

ayshiff Aug 2, 2020

Rugvip left a comment

stefanbuck commented Feb 16, 2021

benjdlambert commented Feb 16, 2021

stefanbuck commented Feb 16, 2021

benjdlambert commented Feb 16, 2021

Refactor: Move @backstage/cli create-app to separate package #1745

Refactor: Move @backstage/cli create-app to separate package #1745

Conversation

ayshiff commented Jul 26, 2020 • edited

Hey, I just made a Pull Request!

✔️ Checklist

Rugvip left a 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

Rugvip Jul 28, 2020 • edited

Choose a reason for hiding this comment

Rugvip left a comment

Choose a reason for hiding this comment

ayshiff commented Jul 31, 2020

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

Rugvip left a comment

Choose a reason for hiding this comment

stefanbuck commented Feb 16, 2021

benjdlambert commented Feb 16, 2021

stefanbuck commented Feb 16, 2021

benjdlambert commented Feb 16, 2021

ayshiff commented Jul 26, 2020 •

edited

Rugvip Jul 28, 2020 •

edited