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

feat: serialised config #1106

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: serialised config #1106

wants to merge 3 commits into from

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Jan 22, 2025

Summary

import { trailingSlash } from "astro:config/client"

console.log(trailingSlash) // logs "never"

Links

@ematipico ematipico marked this pull request as ready for review January 22, 2025 14:35
@ematipico ematipico force-pushed the feat/serialised-config branch from 3ffacf1 to 835486a Compare January 22, 2025 14:38
@ascorbic
Copy link
Contributor

I think this will be very useful. I don't really see the value of exporting them by feature though. Just client or server makes sense to me.

- `publicDir`
- `root`

**By "position"**
Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer by position, it will probably be clearer for users as it will kinda follow the config shape

- `root`

**By "position"**
- `astro:manifest/client`, exported bindings:
Copy link
Member

Choose a reason for hiding this comment

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

The client module should be available on server and client, for usecases like client:load

- `build.format`
- `site`
- `redirects`[^1]
- `astro:manifest/server`, exposed bindings:
Copy link
Member

Choose a reason for hiding this comment

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

I think the server should contain all the data contained in the client module + server only data. I think it's a different situation than astro:env, so the choices made there should not apply here (ie. vars exposed from astro:env/client can be used on the server, but are not exposed on astro:env/server)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the server should contain all the data contained in the client module

Can you provide a rationale for that? An explanation that doesn't involve a comparison with an existing feature

Copy link
Contributor

Choose a reason for hiding this comment

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

I would treat /server as meaning "all serialisable config items that we think could be useful for devs and are happy making a stable API", with /client being the subset that is safe to expose in the browser. If the data might be useful in the client then it might be useful on the server too, so I see no reason to restrict that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just a bit concerned/pedantic on having the same information in two modules.

Do you think it's fine to have them repeated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it's fine. This isn't like z where it's mostly arbitrary: there's only one that can be used in each situation.

Copy link
Member

Choose a reason for hiding this comment

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

there's only one that can be used in each situation.

/client can and should be usable server side, for cases where the code is on both server and client


- The virtual modules will be "usable" via experimental flag. Attempting to use these virtual modules without the experimental flag turned on will result into a hard error.
- After a gracing period where we stabilise the APIs, the experimental flag will be removed.
- We will deprecate existing means to read this information. The following will be deprecated:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also deprecate Astro.site

Copy link
Member Author

Choose a reason for hiding this comment

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

Keep them coming, it's possible I missed some of them

Copy link
Member

Choose a reason for hiding this comment

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

Not deprecated but that also means for actions we won't need the trailing slash placeholder

Copy link
Member Author

Choose a reason for hiding this comment

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

Also the internal i18n utilities

Copy link
Contributor

Choose a reason for hiding this comment

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

It's going to help with a lot of these, and I'm sure all kinds of userland hacks that people have implemented. A much cleaner solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be moved to somewhere else? Mentioning deprecations belongs in Detailed design I would think, but definitely not in Testing Strategy.

@ematipico ematipico changed the title feat: serialised manifest feat: serialised config Jan 24, 2025
@ematipico
Copy link
Member Author

Renamed module to astro:config 8bde590 (#1106)

Proposed sub-paths

**By feature**:
- `astro:config/routing`, exported bindings:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you still proposing these subpaths?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not for this experimental release, but the proposal still there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants