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.

Sign up for GitHub

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

Invert direction of dependency between @faststore/core and @faststore/cli #2377

Merged
merged 15 commits into from
Jul 18, 2024
Merged

Invert direction of dependency between @faststore/core and @faststore/cli #2377

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5cfe1ea
Reorder package.json scripts and rewrite generate as sequence of othe…
gvc Jul 9, 2024
c9dfaae
Remove dependency with @faststore/cli
gvc Jul 9, 2024
cbb18e0
Remove generate:copy-back from core as it should only live inside the…
gvc Jul 9, 2024
fcdc4a6
generate-graphql calls directly `yarn generate` on the tmpDir
gvc Jul 9, 2024
c2d7ed7
Remove unneeded dependency
gvc Jul 9, 2024
5d10fa8
Remove generate-graphql
gvc Jul 9, 2024
08da42d
Split dev into predev, dev and dev-only
gvc Jul 10, 2024
b58abdf
Call predev and dev-only on faststore dev command
gvc Jul 10, 2024
436c990
Add @faststore/core as a dependency of the CLI
gvc Jul 10, 2024
84ee8cf
Revert change to stdio option
gvc Jul 10, 2024
18645f9
Remove dummy code
gvc Jul 11, 2024
c86b1e9
Merge branch 'main' into invert-cli-core-dependency
gvc Jul 11, 2024
2c7f1f4
Merge branch 'main' into invert-cli-core-dependency
gvc Jul 15, 2024
4d81118
Set debug flag to true to keep behavior
gvc Jul 15, 2024
4435bb8
Add message when dev fails to copy generated graphql schema
gvc Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"/oclif.manifest.json"
],
"dependencies": {
"@faststore/core": "^3.0.81",
"@inquirer/prompts": "^5.1.2",
"@oclif/core": "^1.16.4",
"@oclif/plugin-help": "^5",
Expand Down
32 changes: 27 additions & 5 deletions packages/cli/src/commands/dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ import { spawn } from 'child_process';
import chokidar from 'chokidar';
import dotenv from 'dotenv';

import { readFileSync } from 'fs';
import { readFileSync, cpSync } from 'fs';
import path from 'path';
import { withBasePath } from '../utils/directory';
import { generate } from '../utils/generate';
import { runCommandSync } from '../utils/runCommandSync';

/**
* Taken from toolbelt
Expand All @@ -31,10 +32,21 @@ const defaultIgnored = [

const devAbortController = new AbortController()

async function storeDev(rootDir: string, tmpDir: string) {
async function storeDev(rootDir: string, tmpDir: string, coreDir: string) {
const envVars = dotenv.parse(readFileSync(path.join(rootDir, 'vtex.env')))

const devProcess = spawn('yarn dev', {
runCommandSync({
cmd: 'yarn predev',
errorMessage:
'GraphQL was not optimized and TS files were not updated. Changes in the GraphQL layer did not take effect',
Copy link
Contributor

Choose a reason for hiding this comment

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

question: here other commands are run, right? yarn generate:schema && yarn generate:codegen && yarn format:generated not only the yarn generate:codegen. What would happen if only the format returned error? It would fail but the previous commands would have made changes on the files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the changes are made to ephemeral files so, theoretically, no harm is done. Because of throws: 'error' here, the behavior is the same. Whatever step fails there will be no rollback.

throws: 'error',
debug: true,
cwd: tmpDir,
})

copyGenerated(path.join(tmpDir, '@generated'), path.join(coreDir, '@generated'))

const devProcess = spawn('yarn dev-only', {
shell: true,
cwd: tmpDir,
signal: devAbortController.signal,
Expand All @@ -50,6 +62,16 @@ async function storeDev(rootDir: string, tmpDir: string) {
})
}

function copyGenerated(from: string, to: string) {
try {
cpSync(from, to, { recursive: true, force: true })

return { success: true }
} catch (err) {
return { success: false }
Copy link
Contributor

Choose a reason for hiding this comment

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

question: an error to copy would abort the command run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not. But I believe you asked this question because the code is returning this and we're not using it anywhere else. I'll adjust the code to warn the user of the impact of the failure to copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

}
}

export default class Dev extends Command {
static args = [
{
Expand All @@ -62,7 +84,7 @@ export default class Dev extends Command {
const { args } = await this.parse(Dev)
const basePath = args.path ?? process.cwd()

const { getRoot, tmpDir } = withBasePath(basePath)
const { getRoot, tmpDir, coreDir } = withBasePath(basePath)

const queueChange = (/* path: string, remove: boolean */) => {
generate({ basePath })
Expand All @@ -86,7 +108,7 @@ export default class Dev extends Command {

await generate({ setup: true, basePath })

storeDev(getRoot(), tmpDir)
storeDev(getRoot(), tmpDir, coreDir)

return await new Promise((resolve, reject) => {
watcher
Expand Down
83 changes: 0 additions & 83 deletions packages/cli/src/commands/generate-graphql.ts
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions packages/cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { default as Dev } from './commands/dev'
import { default as Build } from './commands/build'
import { default as Serve } from './commands/start'
import { default as CmsSync } from './commands/cms-sync'
import { default as GenerateGraphql } from './commands/generate-graphql'
import { default as Test } from './commands/test'

export const commands = {
Expand All @@ -14,6 +13,5 @@ export const commands = {
build: Build,
serve: Serve,
'cms-sync': CmsSync,
'generate-graphql': GenerateGraphql,
test: Test,
}
14 changes: 7 additions & 7 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,19 @@
"scripts": {
"generate:schema": "tsx src/server/generator/generateGraphQLSchemaFile.ts",
"generate:codegen": "graphql-codegen",
"generate:copy-back": "copyfiles \"@generated/**/*\" $DESTINATION",
"generate": "faststore generate-graphql -c -d",
Copy link
Member

Choose a reason for hiding this comment

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

I miss the -d (debug) flag.
Historically, some clients complain about generic errors in the generated graphql process. This debug flag adds more visibility into the graphql generation flow. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I belive that the flag and better error messages are two different problems, but I'll add this back

Copy link
Member

Choose a reason for hiding this comment

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

I linked these two topics because we previously saw the specific error messages when using the flag.

"build": "yarn partytown & yarn generate && next build",
"dev": "yarn partytown & yarn generate && next dev",
"format:generated": "prettier --write \"@generated/**/*.{ts,js,tsx,jsx,json}\" --loglevel error",
"generate": "yarn generate:schema && yarn generate:codegen && yarn format:generated",
"prebuild": "yarn partytown && yarn generate",
"build": "next build",
"predev": "yarn partytown && yarn generate",
"dev": "next dev",
"dev-only": "next dev",
Comment on lines +21 to +22
Copy link
Member

Choose a reason for hiding this comment

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

the same? Why 2 scripts?
edit: I think this is intentional. So if we need to change, we will change the dev/predev, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I thought I had given you an answer here.

Yes it's intentional and if we need to change something that should happen before the next dev call, we should add it to the predev script, never directly to the dev one.

"clean": "rm -r .next",
"serve": "next start",
"test:e2e": "cypress open",
"test": "jest",
"lhci": "lhci autorun",
"format": "prettier --write \"**/*.{ts,js,tsx,jsx,json}\"",
"format:generated": "prettier --write \"@generated/**/*.{ts,js,tsx,jsx,json}\" --loglevel error",
"lint": "next lint",
"stylelint": "stylelint \"**/*.scss\"",
"stylelint:fix": "stylelint \"**/*.scss\" --fix",
Expand Down Expand Up @@ -60,7 +62,6 @@
"@vtex/prettier-config": "1.0.0",
"autoprefixer": "^10.4.0",
"chalk": "^5.2.0",
"copyfiles": "^2.4.1",
"css-loader": "^6.7.1",
"deepmerge": "^4.3.1",
"draftjs-to-html": "^0.9.1",
Expand All @@ -86,7 +87,6 @@
"devDependencies": {
"@cypress/code-coverage": "^3.12.1",
"@envelop/testing": "^6.0.0",
"@faststore/cli": "^3.0.83",
"@faststore/eslint-config": "^3.0.68",
"@faststore/lighthouse": "^1.12.32",
"@lhci/cli": "^0.9.0",
Expand Down
Loading