-
Notifications
You must be signed in to change notification settings - Fork 28
feat(cli): make app path optional #457
Conversation
| test('get skipped with the `no-installation` flag', () => { | ||
| execSync( | ||
| `yarn start ${appPath} \ | ||
| --name ${appName} \ |
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.
To avoid the prompt, we need to pass the app name explicitly in the tests.
| ]); | ||
| appPath = answers.appPath; | ||
| } | ||
| if (appPath.startsWith('~/')) { |
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.
Unlike command-line argument, the string path including ~/ needs to be expanded by ourselves.
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.
How does it work if it starts with / or ./? Perhaps we should use a Regex instead.
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.
is there a way to forward the path expanding to the shell instead?
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.
If it's a problem, there's also https://nodejs.org/api/path.html#path_path_relative_from_to
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.
@francoischalifour @Haroenv
In the end, it gets resolved here.
> path.resolve('/test')
'/test'
> path.resolve('./test')
'/Users/eunjaelee/workspace/create-instantsearch-app/test'
> path.resolve('~/test')
'/Users/eunjaelee/workspace/create-instantsearch-app/~/test'
>
If a path starts with / or ./, path.resolve() works correctly. However ~ didn't work, because, I guess, it has no context of home directory, or something.
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.
Thanks for checking Eunjae!
|
I was thinking that the name and the directory name probably shouldn't be the same, so that it would be: not sure if that works with what you had in mind. I guess it also makes sense to ask for directory name if it's not given though |
According to Let me know if I misunderstood you. FYI, I've added the example in the PR description. |
Haroenv
left a comment
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.
good for me, with those small suggestions!
Co-authored-by: Haroen Viaene <hello@haroen.me>
Co-authored-by: Haroen Viaene <hello@haroen.me>
Summary
(
npx create-instantsearch-appwill asks for the directory)Examples
Example 1
(path provided)
date-pickeris given as a default name but you can change there.Example 2
(path not provided)
Let's say you typed
date-picker, then the next step would be:The name is inferred from the path but again lets you update it.