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

Container APIs #916

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

Container APIs #916

wants to merge 9 commits into from

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented May 6, 2024

Summary

An API for rendering components in isolation:

// Card.test.js
import Card from "../src/components/Card.astro"
import astroConfig from "../src/astro.config.mjs";

const container = await AstroContainer.create()
const response = await container.renderToString(Card);
// assertions

Links

@ematipico ematipico marked this pull request as ready for review May 6, 2024 13:41
ematipico and others added 2 commits May 8, 2024 15:49
Co-authored-by:  Matthew Phillips <matthew@skypack.dev>
proposals/0048-container-api.md Outdated Show resolved Hide resolved
proposals/0048-container-api.md Outdated Show resolved Hide resolved
Comment on lines +191 to +192
Considering the fact that this API doesn't involve any configuration or virtual module, the API
will be released with a `experimental_` prefix, e.g. `experimental_AstroContainer`.
Copy link
Member

Choose a reason for hiding this comment

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

As a data point, currently Astro's programmatic APIs are experimental too but doesn't have the experimental_ prefix, so I'm biased that we don't need to prefix these and only add an @experimental jsdoc tag. Curious to hear what others think about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this sync and we decided to keep the prefix.

ematipico and others added 4 commits May 15, 2024 14:57
Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
Co-authored-by:  Matthew Phillips <matthew@skypack.dev>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
}
```
The `astroConfig` object is literally the same object exposed by the `defineConfig`, inside the `astro.config.mjs` file. This very configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you found use-case for passing the astroConfig yet? I'm worried about getting bug reports when people try to pass through vite config and other non-supported stuff.

What do you think about instead raising the relevant config values up to the AstroContainerOptions level. That would be stuff like trailingSlash.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about instead raising the relevant config values up to the AstroContainerOptions level. That would be stuff like trailingSlash.

I don't think that would work easily. These options, internally, are used to create a manifest. If we pass these options when rendering a component, it would mean generating a new manifest every time we attempt to render a component, and then discard that manifest. We have to be careful not to override the existing manifest, because the manifest is tight to the lifetime of the instance of the container.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not when rendering, AstroContainerOptions is the name of the options passed to AstroContainer.create().

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I misunderstood what you meant. Yeah we should be able to provide the individual options.

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.

None yet

4 participants