Invert direction of dependency between @faststore/core and @faststore/cli #2377
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
gvc
changed the title
gvc
requested review from
emersonlaurentino and
lariciamota
and removed request for
a team
gvc
force-pushed
the
invert-cli-core-dependency
branch
from
a7c5d36 to
18645f9
Compare
| "dev": "next dev", | ||
| "dev-only": "next dev", |
There was a problem hiding this comment.
the same? Why 2 scripts?
edit: I think this is intentional. So if we need to change, we will change the dev/predev, right?
There was a problem hiding this comment.
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.
| @@ -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", | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I belive that the flag and better error messages are two different problems, but I'll add this back
There was a problem hiding this comment.
I linked these two topics because we previously saw the specific error messages when using the flag.
| runCommandSync({ | ||
| cmd: 'yarn predev', | ||
| errorMessage: | ||
| 'GraphQL was not optimized and TS files were not updated. Changes in the GraphQL layer did not take effect', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
|
||
| return { success: true } | ||
| } catch (err) { | ||
| return { success: false } |
There was a problem hiding this comment.
question: an error to copy would abort the command run?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
LGTM!
It worked as expected!
I've tested in the playground and starter stores.
remove @faststore/core as an explicit dependency,
One thing to pay attention:
It's very uncommon to remove the core dependency because some functions from @faststore/core are used in our features as API extensions, Section Overrides etc.
but I've tested in both scenarios (with and without the core) and it worked :)
but since the CLI depends on Core, it will always be there :) |
What's the purpose of this pull request?
This PR aims to make
@faststore/corea dependency of the@faststore/cli. It just makes sense. Someone can fork@faststore/coreand never need to use the CLI, but there's hardly any usefulness to the CLI without@faststore/core.How it works?
The only thing linking Core to the CLI was the
generate-graphqlcommand on the CLI. It just called back 4 scripts on core'spackage.json:generate:schema,generate:codegen,format:generatedandgenerate:copy-back. The copy-back command was only needed to improve the DX of customers customizing their GraphQL schema [ref] and it was the only part of the script that needed information available at the CLI level only.Since it was only a DX improvement, all that was needed was, during the
faststore devexecution I had to generate the new schema, copy the files to the correctnode_modulesdir, and then start the NextJS dev server. In order to do this I split thedevscript intopredev,dev, anddev-only*. From the FastStore CLI I could now callyarn predev, copy the files to the correct place, and then runyarn dev-onlyto start the NextJS dev server.Someone developing on the faststore monorepo (us VTEX developers, or someone contributing to our project) will continue to run core via
yarn dev. npm, yarn, and pnpm support the lifecycle scripts out of the box. I did the same thing tobuildfor equivalence.One thing that can be up to discussion is the existence of
runCommandSyncutil. It was only used ongenerate-graphqland can be easily removed (in this PR or in a future one).How to test it?
Add this PR
@faststore/clifrom codesandbox as a dependency of the starter store, remove@faststore/coreas an explicit dependency, and@faststore/clias a devDependency. Run your store and see that everything still works fine. Or go to the starter preview [PR] here and see the store running in "production".