Skip to content
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

Fix create-strapi-app for commander 8.2.0 #15060

Merged
merged 1 commit into from Dec 2, 2022

Conversation

innerdvations
Copy link
Contributor

@innerdvations innerdvations commented Dec 2, 2022

What does it do?

Updates the create-strapi-app command to work with commander v8.2.0 (it was previously written using commander v6.1.0)

Why is it needed?

There was a breaking change between commander 6.x -> 7.x that caused the options flags to not be passed through

How to test it?

Run create-strapi-app --quickstart testProjName and it should now generate a quickstart app without prompting for input

Related issue(s)/PR(s)

Issue: #15056

Original PR that introduced the error: #15043

@innerdvations innerdvations added source: cli Source is cli package pr: fix This PR is fixing a bug labels Dec 2, 2022
@innerdvations innerdvations self-assigned this Dec 2, 2022
@alexandrebodin alexandrebodin requested review from Convly and removed request for alexandrebodin December 2, 2022 08:43
@innerdvations
Copy link
Contributor Author

Just noticed we also need to fix create-strapi-starter, working on that now.

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Base: 59.77% // Head: 59.77% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (b12ba7e) compared to base (9cef257).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15060   +/-   ##
=======================================
  Coverage   59.77%   59.77%           
=======================================
  Files        1345     1345           
  Lines       32692    32692           
  Branches     6236     6236           
=======================================
+ Hits        19541    19542    +1     
+ Misses      11289    11288    -1     
  Partials     1862     1862           
Flag Coverage Δ
back 49.90% <ø> (ø)
front 64.27% <ø> (+<0.01%) ⬆️
unit_back 49.90% <ø> (ø)
unit_front 64.27% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...content-manager/components/InputUID/useDebounce.js 100.00% <0.00%> (+10.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

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

Tested create-strapi-app and looks fine to me, thanks!

EDIT: Waiting for create-strapi-starter 👀

@innerdvations
Copy link
Contributor Author

Just noticed we also need to fix create-strapi-starter, working on that now.

Actually, create-strapi-starter seems to work the same as before, the issues I noticed (options present in the code that aren't displayed or used by the command) have always existed. I'll take a look at that later.

@stb13579
Copy link
Contributor

stb13579 commented Dec 2, 2022

Just noticed we also need to fix create-strapi-starter, working on that now.

Actually, create-strapi-starter seems to work the same as before, the issues I noticed (options present in the code that aren't displayed or used by the command) have always existed. I'll take a look at that later.

If we need to expand or improve the documentation, just loop me in and I will take care of it.

@innerdvations
Copy link
Contributor Author

innerdvations commented Dec 2, 2022

Just noticed we also need to fix create-strapi-starter, working on that now.

Actually, create-strapi-starter seems to work the same as before, the issues I noticed (options present in the code that aren't displayed or used by the command) have always existed. I'll take a look at that later.

If we need to expand or improve the documentation, just loop me in and I will take care of it.

I don't have anything specific to say, but if you google "create-strapi-starter" you basically get a blog post, and can also find the Strapi starters page which actually uses "create strapi-app" (app, and without the dash) but doesn't outline any of the options.

Essentially, it feels like "create-strapi-starter" was announced in a blog post but is otherwise undocumented (unless it's buried somewhere searches can't find).

Personally, I'm not sure if more documentation is the path. To me it makes more sense to deprecate it and combine it with create-strapi-app unless there's a specific reason to maintain two commands.

@stb13579
Copy link
Contributor

stb13579 commented Dec 2, 2022

Just noticed we also need to fix create-strapi-starter, working on that now.

Actually, create-strapi-starter seems to work the same as before, the issues I noticed (options present in the code that aren't displayed or used by the command) have always existed. I'll take a look at that later.

If we need to expand or improve the documentation, just loop me in and I will take care of it.

I don't have anything specific to say, but if you google "create-strapi-starter" you basically get a blog post, and can also find the Strapi starters page which actually uses "create strapi-app" (app, and without the dash) but doesn't outline any of the options.

Essentially, it feels like "create-strapi-starter" was announced in a blog post but is otherwise undocumented (unless it's buried somewhere searches can't find).

Personally, I'm not sure if more documentation is the path. To me it makes more sense to deprecate it and combine it with create-strapi-app unless there's a specific reason to maintain two commands.

OK I understand it better now. Regardless if it exists as a separate command or as a series of options it should be in the CLI documentation. I will add it to our backlog with a note that we need to sync with DX on the decision.

@innerdvations innerdvations merged commit e3c273c into main Dec 2, 2022
@innerdvations innerdvations deleted the fix/15056-create-strapi-app-commander-upgrade branch December 2, 2022 16:05
@innerdvations innerdvations added this to the 4.5.4 milestone Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: cli Source is cli package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants