feat(create-docusaurus): ask user for preferred language when no language CLI option provided #9442
Conversation
Rafael-Martins
requested review from
slorber,
lex111 and
Josh-Cena
as code owners
|
Hi @Rafael-Martins! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Rafael-Martins
changed the title
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Thanks
It's probably working but the code flow is not super clear, I'd appreciate if you refactor it a bit more.
For example getSource should probably get a clear language: "javascript" | "typescript" attribute instead of overridden cli options
Not sure how hard this would be but if there's only a JS template option and no TS variant available for that template, maybe we could avoid the extra prompt because we'll ask an extra useless question
| let language: {javascript?: boolean; typescript?: boolean} = {}; | ||
| if (!cliOptions.typescript && !cliOptions.javascript) { | ||
| const {language: selectedLanguage} = (await prompts( | ||
| { | ||
| type: 'select', | ||
| name: 'language', | ||
| message: 'What language you want to use?', | ||
| choices: [ | ||
| {title: 'JavaScript', value: 'javascript'}, | ||
| {title: 'TypeScript', value: 'typescript'}, | ||
| ], | ||
| }, | ||
| { | ||
| onCancel() { | ||
| logger.info`Falling back to language=${'javascript'}`; | ||
| }, | ||
| }, | ||
| )) as {language: keyof Languages}; | ||
|
|
||
| language = {[selectedLanguage]: true}; | ||
| } |
There was a problem hiding this comment.
Could you encapsulate all this under an abstraction?
Something like:
async function getLanguage(languages: LanguagesOptions): Promise<"javascript" | "typescript">We'll likely use in other places in the future, cf swizzle CLI:
#9402
So that's worth extracting this to docusaurus-utils > cliUtils.ts immediately IMHO so that we'll be able to reuse it in other places
extract language logic into cliUtils consider template to decide if the prompt will show or not
|
Hey @slorber, I addressed the comments above, let me now if we can improve it even more! |
| typescript?: boolean; | ||
| }; | ||
|
|
||
| export async function getLanguage( |
There was a problem hiding this comment.
This function is only used in create-docusaurus. Please don't put it in utils. (You even had to add an entire prompts dependency.)
There was a problem hiding this comment.
Hello, I carried out this extraction in response to @slorber's comment, which suggested that it might be reused in another part of the code. If that's the case, I'm considering passing the 'prompt' function as a parameter to eliminate the dependency. What are your thoughts on this idea? I'm also open to the possibility of reintegrating it into 'create-docusaurus' if that's a better solution.
There was a problem hiding this comment.
yes, considering we also want this for swizzle it's fine to add it to docusaurus utils
see also related pr: #9681
slorber
added
pr: new feature
| typescript?: boolean; | ||
| }; | ||
|
|
||
| export async function getLanguage( |
There was a problem hiding this comment.
yes, considering we also want this for swizzle it's fine to add it to docusaurus utils
see also related pr: #9681
| if (noTsVersionAvailable) { | ||
| return {javascript: true}; | ||
| } |
There was a problem hiding this comment.
I'm not sure it's a good thing to have this thing here, will try to see where to move it
|
Thanks 👍 I took the opportunity to refactor a bit the CLI because it was quite messy 🤪 Also adjusting the way we prompt for the language choice because we are going to share some code with another related PR: #9681 will be in v3.2 |
slorber
changed the title
Pre-flight checklist
Motivation
Following the issue #9401, I created a prompt that just shows up when neither
--javascriptor--typescriptis passedUsage screenshot
Related issues/PRs
#9401