-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
feat(cli): connect Vuejs project with client's cloud org #43
Conversation
Pull Request Report PR Title ✅ Title follows the conventional commit spec. |
private createProject(name: string, preset: {}) { | ||
return this.runVueCliCommand([ | ||
'create', | ||
name, | ||
'--inlinePreset', | ||
JSON.stringify(preset), | ||
'--skipGetStarted', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does --skipGetStarted and --bare options do ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--bare
: Scaffold project without beginner instructions on the home page. There is no point in having them because we override the home page with the search page.
--skipGetStarted
: Skip displaying "Get started" instructions. We don't want the create
command to display the To get started, cd myproject && yarn serve
message because the Coveo template was not invoked yet. The creation of a Coveo project is a 2 step process:
vue create project
vue invoke @coveo/typescript
Ideally, we want to display a nice get-started message after our template is invoked.
const {token} = await res.json(); | ||
const engineService = new EngineService(token); | ||
// Adding Coveo Headless engine as a global mixin so it can be available to all components | ||
Vue.mixin(engineService.mixin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat, did not know about that vue feature !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some change and questions :)
} | ||
|
||
function isEnvFile(path) { | ||
return path.split(sep).indexOf('.env.example') !== -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this function.
The condition for a file to be an envFile would be imo:
/.env$/.test(path);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll rename it to be isExampleEnvFile
.
The goal is to copy everything but the env.example file.
And that's because the .env
is define at the root of the entire project (not at the server level).
I'm creating the .env file later in the process
function startServer() { | ||
const serverPath = join(process.cwd(), 'server'); | ||
const child = spawnSync('npm', ['run', 'start'], { | ||
stdio: 'inherit', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we OK with mixing up stdio of npm run serve and the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we want to make it easy for the user.
The other option would be to run 2 npm scripts (one for the web app and one for the token server).
@@ -0,0 +1,47 @@ | |||
/* eslint-disable @typescript-eslint/no-non-null-assertion */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 hmmm.
I know that's a 🍞 but, I'd adheres to our eslint and change the
l25 organizationId: process.env.VUE_APP_ORGANIZATION_ID!,
with the appropriate pre-checks. (e.g. you could have a function isEnvValid
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a great idea. We can do that check at the router level and redirect the user to an error page if the .env file is not valid. I think that is better than just printing errors in the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the env check but the linting could be part of different PR (CDX-112)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
No description provided.