[legacy-framework] [#330] Fix new app command order #367

peaonunes · 2020-04-30T11:45:03Z

Type: bug fix

Closes: blitz-js/legacy-framework#506

What are the changes and their implications? ⚙️

Git init happens before any other step in AppGenerator.
Husky hooks will now correctly be applied when commiting files.

Checklist

Tests added for changes
Any added terminal logging uses packages/server/src/log.ts

Breaking change: no

Other information

I've took the opportunity to add tests as there were none before. I had to do some tricky mocks though, not super happy.

Locally when I run yarn test in the root folder in the project the cli tests fail. However, when running inside the cd packages/cli && yarn test everything is green. Does anyone know what is the issue?
I had to ignore the test hooks because they were not allowing me to push the code.

peaonunes · 2020-04-30T11:46:24Z

packages/cli/src/generators/app.ts

-      } else {
-        console.error('Failed to run git add')
-      }
+    if (gitInitResult.status === 0) {


Curious on the motivation behind not failing if the git commands fail. If we want to i am happy to make them throw error here as well as the other commands inside this generator.

I'm not super set on failing if git init fails just because it's not a mandatory part of the setup process. I'll open an issue to discuss this more though, thanks for bringing it up!

peaonunes · 2020-04-30T12:20:37Z

packages/cli/test/generators/app.test.ts

+        await generator.run()
+      } catch (e) {
+        expect(e).toEqual(new Error('Failed to yarn install'))
+      }


Tried hard to make this work with toThrow, but seems to be a known issue.
jestjs/jest#1700

peaonunes · 2020-04-30T12:23:06Z

packages/cli/src/generators/app.ts

@@ -64,7 +68,7 @@ class AppGenerator extends Generator<AppGeneratorOptions> {

    const result = spawn.sync(this.options.yarn ? 'yarn' : 'npm', ['install'], {stdio: 'inherit'})
    if (result.status !== 0) {
-      throw new Error()
+      throw new Error(`Failed to ${this.options.yarn ? 'yarn' : 'npm'} install`)
    }

    console.log(chalk.hex(themeColor).bold('\nDependencies successfully installed.'))


Curious why we are using log helper and/or console.log|console.error. Should it be unified?

I've been incrementally migrating to the @blitzjs/server/utils/log helper as I touch relelvant lines. We haven't touched the app generator meaningfully in a bit so I haven't had a chance to migrate these log lines. If you want to migrate these to use the main logging utility it would be a huge help!!

Done c31ba55

aem · 2020-04-30T14:53:16Z

Thanks so much for the PR! I hadn't noticed this, great find + fix!

Locally when I run yarn test in the root folder in the project the cli tests fail. However, when running inside the cd packages/cli && yarn test everything is green. Does anyone know what is the issue?

This is a known issue, the tests run in parallel in the prepush hook but there's a race condition there :( Since we reuqire a successful build to merge and the CI build runs the tests serially it's ok to ignore them locally, there's no chance it'll make it into the canary branch.

aem · 2020-04-30T14:55:52Z

packages/cli/src/generators/app.ts

@@ -64,7 +68,7 @@ class AppGenerator extends Generator<AppGeneratorOptions> {

    const result = spawn.sync(this.options.yarn ? 'yarn' : 'npm', ['install'], {stdio: 'inherit'})
    if (result.status !== 0) {
-      throw new Error()
+      throw new Error(`Failed to ${this.options.yarn ? 'yarn' : 'npm'} install`)


Since we now use the yarn/npm split in a few places, would it be possible to pull that logic out into a constant? It'll prevent any chance of us (most likely me 😬) messing it up in the future

Done 77d4a5c

aem · 2020-04-30T14:58:02Z

packages/cli/test/generators/app.test.ts

@@ -0,0 +1,237 @@
+import * as fs from 'fs-extra'


Thank you so much for adding some test scaffolding here! I think @dajinchu added an in-memory filesystem to Jest in the server package, maybe we could follow a similar pattern here so we could actually generate the app for real to make sure things work ok?

I focused on unit tests here so I tried to mock all the external integrations. I believe we may want to have a good amount of both unit and integration tests.
However, we could add those integrations tests in a different suit file to not conflict setup etc. What do you think of us having some sort of app.int.test.ts (I am bad at naming) or even a different folder cli/test/integration?
I am happy to add these as a new PR so we incrementally improve the quality of the tests.

Yeah we used mock-fs which worked pretty well.

aem · 2020-05-01T00:46:07Z

packages/cli/src/generators/app.ts

+    for (let command of commands) {
+      const result = spawn.sync(...command)
+      if (result.status !== 0) {
+        console.error(`Fialed to run command ${command[0]} with ${command[1].join(' ')} options.`)


Suggested change

console.error(`Fialed to run command ${command[0]} with ${command[1].join(' ')} options.`)

console.error(`Failed to run command ${command[0]} with ${command[1].join(' ')} options.`)

Done 77d4a5c

peaonunes · 2020-05-01T05:51:19Z

New changes were added while this PR is being reviewed, I will take a time to rebase and fix the conflicts.

flybayer · 2020-05-01T07:25:46Z

Thank you @peaonunes! I'll mark as draft — mark it as ready for review once it's updated.

@all-contributors add @peaonunes for code

allcontributors · 2020-05-01T07:25:57Z

@flybayer

I've put up a pull request to add @peaonunes! 🎉

peaonunes · 2020-05-01T09:22:42Z

Hey @flybayer thanks, I've updated now.
@aem this is ready for review again.

Unfortunately seems like a lot have changed from when I started adding tests and refactoring some calls. So I had to undo those. In order to avoid this happening again I've only made the change needed on this PR and added tests related to it. Happy on refactor again portions of this code in the future.

flybayer · 2020-05-02T02:02:46Z

@peaonunes did you see your tests are failing?

peaonunes · 2020-05-02T02:21:16Z

Hey yeah. Trying to figure out why, its complaining about using wothBrand. My changes have nothing to do with the failing point, but at the same time the code is up to date with canary. I might start fresh from canary again in a few hours.

peaonunes · 2020-05-02T07:30:16Z

Hey, everyone.
Closing this PR , I sorted everything out in this new one #399.
Sorry for the mess.
cc: @aem @flybayer

peaonunes requested a review from aem as a code owner April 30, 2020 11:45

peaonunes commented Apr 30, 2020

View reviewed changes

aem reviewed Apr 30, 2020

View reviewed changes

aem reviewed May 1, 2020

View reviewed changes

aem mentioned this pull request Jul 7, 2022

Add helpful message about installing git during blitz new if it doesn't exist blitz-js/legacy-framework#489

Closed

peaonunes requested a review from aem May 1, 2020 01:12

flybayer marked this pull request as draft May 1, 2020 07:25

allcontributors bot mentioned this pull request May 1, 2020

[legacy-framework] docs: add peaonunes as a contributor #382

Merged

Adds git init before any any install

5410aa0

peaonunes marked this pull request as ready for review May 1, 2020 09:17

Merge branch 'canary' into fix-new-app-command-order

2e8f5a6

peaonunes added 4 commits May 1, 2020 19:25

Adds log.withBrand back

a010c4c

Merge branch 'canary' into fix-new-app-command-order

215111e

Merge branch 'canary' into fix-new-app-command-order

abaa18a

Merge branch 'canary' into fix-new-app-command-order

3f21428

simondebbarma added the status/in-review label May 1, 2020

Merge branch 'canary' into fix-new-app-command-order

c4a6813

peaonunes closed this May 2, 2020

simondebbarma added kind/bug Something isn't working status/done and removed status/in-review labels May 2, 2020

agilgur5 mentioned this pull request Jul 7, 2022

Support separate client and server bundles jaredpalmer/tsdx#751

Closed

dillondotzip changed the title ~~[#330] Fix new app command order~~ [legacy-framework] [#330] Fix new app command order Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[legacy-framework] [#330] Fix new app command order #367

[legacy-framework] [#330] Fix new app command order #367

peaonunes commented Apr 30, 2020

peaonunes Apr 30, 2020

aem Apr 30, 2020

peaonunes Apr 30, 2020

peaonunes Apr 30, 2020

aem Apr 30, 2020

peaonunes May 1, 2020

aem commented Apr 30, 2020

aem Apr 30, 2020

peaonunes May 1, 2020

aem Apr 30, 2020

peaonunes May 1, 2020

dajinchu May 2, 2020

aem May 1, 2020

peaonunes May 1, 2020

peaonunes commented May 1, 2020

flybayer commented May 1, 2020

allcontributors bot commented May 1, 2020

peaonunes commented May 1, 2020

flybayer commented May 2, 2020

peaonunes commented May 2, 2020

peaonunes commented May 2, 2020

	console.error(`Fialed to run command ${command[0]} with ${command[1].join(' ')} options.`)
	console.error(`Failed to run command ${command[0]} with ${command[1].join(' ')} options.`)

[legacy-framework] [#330] Fix new app command order #367

[legacy-framework] [#330] Fix new app command order #367

Conversation

peaonunes commented Apr 30, 2020

Type: bug fix

What are the changes and their implications? ⚙️

Checklist

Breaking change: no

Other information

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

Choose a reason for hiding this comment

aem commented Apr 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peaonunes commented May 1, 2020

flybayer commented May 1, 2020

allcontributors bot commented May 1, 2020

peaonunes commented May 1, 2020

flybayer commented May 2, 2020

peaonunes commented May 2, 2020

peaonunes commented May 2, 2020