Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

@eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Jun 23, 2021

Summary

  • This PR makes argument optional.
    (npx create-instantsearch-app will asks for the directory)
  • After inferring project name from the directory, it asks if you want to name it differently.

Examples

Example 1

(path provided)

$ npx create-instantsearch-app ~/workspace/date-picker

The name of the application or widget: [date-picker]

date-picker is given as a default name but you can change there.

Example 2

(path not provided)

$ npx create-instantsearch-app

Project directory: 

Let's say you typed date-picker, then the next step would be:

The name of the application or widget: [date-picker]

The name is inferred from the path but again lets you update it.

test('get skipped with the `no-installation` flag', () => {
execSync(
`yarn start ${appPath} \
--name ${appName} \
Copy link
Contributor Author

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('~/')) {
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking Eunjae!

@eunjae-lee eunjae-lee requested review from a team, Haroenv, francoischalifour and tkrugg and removed request for a team and tkrugg June 23, 2021 13:42
@Haroenv
Copy link
Contributor

Haroenv commented Jun 23, 2021

I was thinking that the name and the directory name probably shouldn't be the same, so that it would be:

Create InstantSearch App
[ errors ]

Create InstantSearch App m-directory
> widget name? [myDirectory]

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

@eunjae-lee
Copy link
Contributor Author

I was thinking that the name and the directory name probably shouldn't be the same

According to my-directory and myDirectory, do you mean the case difference? the name variable is not camelCase and we have a dedicated camelCaseName variable.

Let me know if I misunderstood you.

FYI, I've added the example in the PR description.

Copy link
Contributor

@Haroenv Haroenv left a 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>
@eunjae-lee eunjae-lee merged commit d5a5655 into master Jun 28, 2021
@eunjae-lee eunjae-lee deleted the feat/optional-path branch June 28, 2021 08:37
aymeric-giraudet pushed a commit to algolia/instantsearch that referenced this pull request Dec 14, 2022
Co-authored-by: Haroen Viaene <hello@haroen.me>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants