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

adding a new flag --mode for dev and pro #6735

Closed
wants to merge 26 commits into from

Conversation

JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Apr 3, 2023

Changes

Fix #6575
adding a new --mode flag for dev and pro.
such as dev pnpm run dev --mode=test
such as build pnpm run pnpm --mode=test

Testing

Docs

@changeset-bot
Copy link

changeset-bot bot commented Apr 3, 2023

🦋 Changeset detected

Latest commit: 22f8e79

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Apr 3, 2023
@JerryWu1234 JerryWu1234 marked this pull request as ready for review April 3, 2023 08:13
@JerryWu1234 JerryWu1234 requested a review from a team as a code owner April 3, 2023 08:13
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Things that we should cover before merging the PR:

  • add the new flag to the CLI (help section);
  • make some manual testing check via CLI to make sure it works;
  • make sure that this new flag is documented in docs.astro.build

packages/astro/src/core/dev/container.ts Outdated Show resolved Hide resolved
packages/astro/test/astro-mode-envs.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-mode-envs.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-mode-envs.test.js Outdated Show resolved Hide resolved
packages/astro/test/astro-mode-envs.test.js Outdated Show resolved Hide resolved
.changeset/odd-foxes-invent.md Outdated Show resolved Hide resolved
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just a heads up that if we're adding a new flag to the CLI, we would also need to document it here: https://docs.astro.build/en/reference/cli-reference/

@JerryWu1234
Copy link
Contributor Author

Things that we should cover before merging the PR:

  • make some manual testing check via CLI to make sure it works;

What are you referring to recording a video?

@ematipico
Copy link
Member

Things that we should cover before merging the PR:

  • make some manual testing check via CLI to make sure it works;

What are you referring to recording a video?

It's not necessary unless you want to! Just a mention that you manually tested your new change I think it's enough!

We don't test the CLI directly in our testing suite, so it's always best to test it locally to ensure it works as expected.

@JerryWu1234
Copy link
Contributor Author

Things that we should cover before merging the PR:

  • make some manual testing check via CLI to make sure it works;

What are you referring to recording a video?

It's not necessary unless you want to! Just a mention that you manually tested your new change I think it's enough!

We don't test the CLI directly in our testing suite, so it's always best to test it locally to ensure it works as expected.

I got it, of course I did

@ematipico
Copy link
Member

I got it, of course I did

Good to know :) next time you can add this information in the template, under the "Testing" section

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

This would be a minor change.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Looks good to me! We keep it blocked until we are ready release a new minor. @wulinsheng123 Did you create a PR in the docs repo with the new flag?

package.json Outdated Show resolved Hide resolved
@JerryWu1234
Copy link
Contributor Author

Looks good to me! We keep it blocked until we are ready release a new minor. @wulinsheng123 Did you create a PR in the docs repo with the new flag?
ok

@sarah11918
Copy link
Member

Just checking in on whether this is planned to be accepted.

An issue has been filed in the docs repo simply stating that we will need documentation to accompany this, but no documentation has been proposed. (Issue: withastro/docs#3024 )

Docs can step in and propose documentation, but we'd rather know that this is going to be accepted first before we do that. 😄

@ematipico
Copy link
Member

ematipico commented May 4, 2023

Happy to merge! @wulinsheng123 could you please update the PR? There's a file that has conflicts

@matthewp
Copy link
Contributor

matthewp commented May 4, 2023

Let's hold off on this one for now. I would like to understand the use-case better. It appears to set some internal code that was never intended to be exposed externally.

@JerryWu1234
Copy link
Contributor Author

Let's hold off on this one for now. I would like to understand the use-case better. It appears to set some internal code that was never intended to be exposed externally.

Ok, if there is any progress, please tell me because I need to resolve a conflict.

@Princesseuh
Copy link
Member

@matthewp Have you had time to revisit this?

@bluwy
Copy link
Member

bluwy commented May 17, 2023

I was probably the one who pushed for it, as I've seen this work on & off in the past in Astro. I think it makes sense to support custom --mode so users can e.g. build a site for staging mode.

I notice in Astro internals, we only differentiate between development and production mode, so we'd probably have to be careful that e.g. staging mode still builds in production mode. At retrospect maybe this should be discussed first 😬

@ematipico
Copy link
Member

I was probably the one who pushed for it, as I've seen this work on & off in the past in Astro. I think it makes sense to support custom --mode so users can e.g. build a site for staging mode.

I notice in Astro internals, we only differentiate between development and production mode, so we'd probably have to be careful that e.g. staging mode still builds in production mode. At retrospect maybe this should be discussed first 😬

I see the use case, but Astro is a bit different and we need to clearly set user expectations of what a --mode is. As far as I understand, development mode is the actual dev server, which differs from the actual production where the pages are actually built into static files (or SSR).

So if the user set a --mode=STAGING, what is it going to affect? What's the expected result in the dev server and in the built artefacts?

@bluwy
Copy link
Member

bluwy commented May 17, 2023

Largely (and maybe only?) it'll affect the value of import.meta.env.MODE which can be used to render things differently. If --mode=staging, I'd expect the dev server to still run in development but with a different import.meta.env.MODE, similarly in build it runs in production with a different import.meta.env.MODE.

The mode name is a bit overloaded with this now though, maybe we should refer --mode as userMode that's used to achieve import.meta.env.MODE and passing that value to Vite's mode.

@matthewp
Copy link
Contributor

Given there's still some unknowns of how/if this should work, I think this warrants a (probably small) RFC where we work through the ideas. We can then reopen this PR if it turns out to be the approach chosen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Env Variables are invalid in version 2.1
6 participants