Skip to content

Fixing type to align with Vite configuration#8193

Closed
gplusdotgr wants to merge 2 commits intowithastro:mainfrom
gplusdotgr:fixing-type-on-server-open
Closed

Fixing type to align with Vite configuration#8193
gplusdotgr wants to merge 2 commits intowithastro:mainfrom
gplusdotgr:fixing-type-on-server-open

Conversation

@gplusdotgr
Copy link
Copy Markdown

@gplusdotgr gplusdotgr commented Aug 22, 2023 •
edited
Loading

Changes

Testing

Docs

@gplusdotgr gplusdotgr requested a review from a team as a code owner August 22, 2023 16:44
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Aug 22, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 0cbc1fd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 22, 2023
@gplusdotgr gplusdotgr mentioned this pull request Aug 22, 2023
Closed
1 task
bluwy
bluwy suggested changes Aug 23, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

This doesn't fix the issue yet. We need to update the schema to allow passing strings:

astro/packages/astro/src/core/config/schema.ts

Line 144 in a571a1a

open: z.boolean().optional().default(ASTRO_CONFIG_DEFAULTS.server.open),

And make sure we're also using the string:

astro/packages/astro/src/core/dev/container.ts

Lines 59 to 64 in a571a1a

const {
base,
server: { host, headers, open: shouldOpen },
} = settings.config;
// Open server to the correct path
const open = shouldOpen ? base : false;

And also that the CLI accepts it:

astro/packages/astro/src/cli/flags.ts

Line 24 in a571a1a

open: typeof flags.open === 'boolean' ? flags.open : undefined,

@gplusdotgr
Copy link
Copy Markdown
Author

gplusdotgr commented Aug 23, 2023

Quick question, @bluwy , if this is a core vite feature, why do we need to override / rewrite with our own logic? (I'm sorry if this is a dumb question, just figuring things out)

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Aug 23, 2023

I don't have the full context, but it's likely because we want to present the dev server as Astro's dev server, and not Vite's. Vite would be an implementation detail. So common server options like host and port are top-level in the Astro config, but if you want to tap into Vite specific server options, like setting up proxies, then you'd have to use the vite.server config.

@gplusdotgr
Copy link
Copy Markdown
Author

gplusdotgr commented Aug 23, 2023

I see. I'm not sure I have the experience in terms of working with the core of the FW to do all the changes required..

@sarah11918 sarah11918 removed the request for review from a team September 13, 2023 13:59
@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Sep 21, 2023

Is this ready for another review? Or still being worked on?

@gplusdotgr
Copy link
Copy Markdown
Author

gplusdotgr commented Sep 22, 2023

@matthewp I don't think I can go through with this, some parts of the codebase are a little blackbox to me.

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Sep 22, 2023

I'll close this for now to clean up the PRs and track the progress in the issue. Thanks for the PR to kick off the discussion

@bluwy bluwy closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bluwy bluwy bluwy requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Astro config conflict / misalignment with Vite config on server.open expected type ( boolean vs boolean & string)

3 participants

@gplusdotgr @bluwy @matthewp