Fixes withastro/astro#7793 astro add cli pass down arguments to install cmd #7794

Elod-T · 2023-07-25T15:08:28Z

Changes

get arguments from the astro add cli
pass them down to the actual package manager

Testing

I tested it locally, running astro add astro-compress -D ran pnpm add astro-compress -D true

Docs

I think it should be in the docs

/cc @withastro/maintainers-docs for feedback!

changeset-bot · 2023-07-25T15:08:32Z

🦋 Changeset detected

Latest commit: 613cad9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ematipico · 2023-07-25T15:26:42Z

No tests, I didn't find any tests for the astro add cli

You could test locally and see if it works as expected:

build your branch with pnpm build
create an empty astro project in another directory
use your package manager to run pnpm link ../astro/packages/astro (path to your internal fork of astro)
run pnpm astro add command (or create a script) with the flags that needs to be passed

That's how I usually test the CLI

ematipico

Thank you for your fix! The changelog is missing and should be added

ematipico · 2023-07-25T15:27:27Z

pnpm-lock.yaml

@@ -18666,7 +18666,6 @@ packages:
    resolution: {directory: packages/astro, type: directory}


These changes should be reverted.

I think it's caused by the latest pnpm version

bluwy · 2023-07-25T15:41:48Z

packages/astro/src/cli/add/index.ts

+	const inheritedFlags = Object.entries(flags)
+		.map(([flag, value]) => {
+			if (flag == '_') return;
+
+			return [`-${flag}`, value.toString()];
+		})
+		.filter(Boolean)
+		.flat();


I don't think we should pass everything down. To be safe, we can scope to --save-dev, -D etc only. In case we support more options in the future, this part won't be glossed over.

I don't get why we should only pass those down. It's essentially giving back the normal functionality with flags to the cli. It will work as pnpm without the astro add wrapper. Please let me know if I am missing something

I think astro add shouldn't be seen as pnpm add but smarter. It does a lot more like editing configs, adding integration-specific configs etc. The installation phase is one of the steps for astro add. So passing the args directly to that single step is a bit off to me.

I agree with @bluwy that we should only support --save-dev and -D for now.

Elod-T · 2023-07-25T20:41:04Z

@ematipico Yes I did test it locally, sorry I wasn't clear enough, I meant no unit tests. I'll make the changelog tomorrow

ematipico · 2023-07-26T11:02:37Z

@ematipico Yes I did test it locally, sorry I wasn't clear enough, I meant no unit tests. I'll make the changelog tomorrow

Could you please update the template, then? It's important to state that you tested the feature. Even a manual test is important.

natemoo-re

Code looks good but I'd also like to reduce the scope of this change to only allow -D and --save-dev for now.

Also if you could remove/resolve the lockfile changes, that would be great.

natemoo-re · 2023-08-01T16:50:20Z

packages/astro/src/cli/add/index.ts

+	const inheritedFlags = Object.entries(flags)
+		.map(([flag, value]) => {
+			if (flag == '_') return;
+
+			return [`-${flag}`, value.toString()];
+		})
+		.filter(Boolean)
+		.flat();


I agree with @bluwy that we should only support --save-dev and -D for now.

Elod-T · 2023-08-02T10:08:26Z

I'm working on the pr, and I'm thinking that we could allow all flags supported by npm install.

-P, --save-prod: Package will appear in your dependencies. This is the default unless -D or -O are present.

-D, --save-dev: Package will appear in your devDependencies.

-O, --save-optional: Package will appear in your optionalDependencies.

--no-save: Prevents saving to dependencies.

When using any of the above options to save dependencies to your package.json, there are two additional, optional flags:

-E, --save-exact: Saved dependencies will be configured with an exact version rather than using npm's default semver range operator.

-B, --save-bundle: Saved dependencies will also be added to your bundleDependencies list.

Source

natemoo-re · 2023-08-10T18:53:02Z

Closing in favor of #8032 so we can get this merged! Unfortunately I wasn't able to push to your fork so I had to push your commits to one of our branches.

astro add cli pass down arguments to install cmd

3941e6f

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jul 25, 2023

ematipico requested changes Jul 25, 2023

View reviewed changes

bluwy reviewed Jul 25, 2023

View reviewed changes

add changeset

613cad9

natemoo-re linked an issue Aug 1, 2023 that may be closed by this pull request

astro add cli doesn't inherit flags #7793

Closed

1 task

natemoo-re requested changes Aug 1, 2023

View reviewed changes

natemoo-re mentioned this pull request Aug 10, 2023

Support flags for astro add #8032

Merged

natemoo-re closed this Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes withastro/astro#7793 astro add cli pass down arguments to install cmd #7794

Fixes withastro/astro#7793 astro add cli pass down arguments to install cmd #7794

Elod-T commented Jul 25, 2023 •

edited

Loading

changeset-bot bot commented Jul 25, 2023 •

edited

Loading

ematipico commented Jul 25, 2023 •

edited

Loading

ematipico left a comment

ematipico Jul 25, 2023

bluwy Jul 25, 2023

bluwy Jul 25, 2023

Elod-T Jul 25, 2023

bluwy Jul 26, 2023

natemoo-re Aug 1, 2023

Elod-T commented Jul 25, 2023

ematipico commented Jul 26, 2023

natemoo-re left a comment •

edited

Loading

natemoo-re Aug 1, 2023

Elod-T commented Aug 2, 2023

natemoo-re commented Aug 10, 2023

		@@ -18666,7 +18666,6 @@ packages:
		resolution: {directory: packages/astro, type: directory}

Fixes withastro/astro#7793 astro add cli pass down arguments to install cmd #7794

Fixes withastro/astro#7793 astro add cli pass down arguments to install cmd #7794

Conversation

Elod-T commented Jul 25, 2023 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Jul 25, 2023 • edited Loading

🦋 Changeset detected

ematipico commented Jul 25, 2023 • edited Loading

ematipico left a comment

Choose a reason for hiding this comment

ematipico Jul 25, 2023

Choose a reason for hiding this comment

bluwy Jul 25, 2023

Choose a reason for hiding this comment

bluwy Jul 25, 2023

Choose a reason for hiding this comment

Elod-T Jul 25, 2023

Choose a reason for hiding this comment

bluwy Jul 26, 2023

Choose a reason for hiding this comment

natemoo-re Aug 1, 2023

Choose a reason for hiding this comment

Elod-T commented Jul 25, 2023

ematipico commented Jul 26, 2023

natemoo-re left a comment • edited Loading

Choose a reason for hiding this comment

natemoo-re Aug 1, 2023

Choose a reason for hiding this comment

Elod-T commented Aug 2, 2023

natemoo-re commented Aug 10, 2023

Elod-T commented Jul 25, 2023 •

edited

Loading

changeset-bot bot commented Jul 25, 2023 •

edited

Loading

ematipico commented Jul 25, 2023 •

edited

Loading

natemoo-re left a comment •

edited

Loading