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

Export experimental JS API #7979

Merged
merged 12 commits into from Aug 18, 2023
Merged

Export experimental JS API #7979

merged 12 commits into from Aug 18, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 7, 2023

Changes

Export dev, build, preview, and sync JavaScript APIs.

Testing

Existing tests updated to use the entrypoint to test it.

Docs

I copied each APIs jsdoc from the CLI reference docs because I am lazy wanted to keep it consistent with what we have now.

We'll likely need a dedicated page for it as we add more APIs in the future (e.g. there was discussion to move defineConfig to the main entrypoint), but it could be weird to have two places referring to similar info?

Appreciate some feedback on this, cc @withastro/maintainers-docs!

EDIT: First draft for docs: withastro/docs#4234

@bluwy bluwy requested a review from a team as a code owner August 7, 2023 14:05
@changeset-bot
Copy link

changeset-bot bot commented Aug 7, 2023

🦋 Changeset detected

Latest commit: a78df6a

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 Aug 7, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

This looks great! Would definitely prefer having a public wrapper for build and sync though.

packages/astro/src/core/build/index.ts Outdated Show resolved Hide resolved
packages/astro/src/core/sync/index.ts Outdated Show resolved Hide resolved
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.

In the AstroInlineConfig, there's a configFile type. We should document it and explain how it behaves. I check its type and I don't remember why there's a string | false.

@bluwy
Copy link
Member Author

bluwy commented Aug 8, 2023

Forgot to document those, just pushed a commit adding some jsdocs.

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.

I still think we need to write some examples and tests for these APIs, to show how they work and if they work as intended.

Maybe we can do a follow-up PR, but I think they are really important

@bluwy
Copy link
Member Author

bluwy commented Aug 8, 2023

I think we're already using them in the tests? I updated test-utils.js to use those. We could use more examples in the docs, but awaiting if the docs team have ideas on how to document those.

@delucis delucis mentioned this pull request Aug 12, 2023
1 task
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Hi @bluwy! Sorry it took me so long to get to this one.

The individual JSDoc comments here look OK in general but I’m missing example of how this would be used and if this is intended to be user-facing or not. Currently the changelog make it sound like an internal refactor.

While we think about where best to document this on the docs site itself, the changelog could be a good opportunity to sketch out some examples of running the commands and using the results?

One other comment from reading through this is that the config behaviour is not entirely clear to me. It seemed from the types that I can pass configuration directly inline, but then there’s also the comment about automatically looking for a config file or passing the path. Not clear to me if these are both the case, and if they are what happens if you have a config file and inline config. Probably something else that examples would clarify.

@bluwy
Copy link
Member Author

bluwy commented Aug 14, 2023

No worries! I think we want this to be user-facing eventually, but it needs more polish along the way. Right now the return results of the functions are a bit lacking and will likely change, so leaning on not documenting those yet. BUT I think the parameters are more stable and are less likely to change.

I should also note in the changelog that they are experimental, and I can add more examples there.

One other comment from reading through this is that the config behaviour is not entirely clear to me.

I'll also clarify this in the changeset! Inline configs should always take the highest priority, and you can indeed specify a custom config path, not load any config, or let Astro searches by default.

@bluwy bluwy changed the title Export JS API Export experimental JS API Aug 14, 2023
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks Bjorn — this is great! Left a few comments.

One bit of API feedback to think about:

Is it necessary to have the complexity of inline config plus file loading plus merging?

I’m trying to think of the scenarios and it seems you’d either use an on-disk config OR provide inline config, but not both. (In cases where you want some shared config which is overridden for a few values, I’d assume you can just import the shared config and spread it in?)

Almost like the usage can be:

// Looks up config on disk
dev();

// Looks up config from custom location on disk
dev({ configFile: './custom-config.js' });

// Uses config inline
dev({ config: { site: 'https://example.com' } });

The high flexibility in the current design may seem user-friendly — they can do whatever they want! — but sometimes that leads to unintuitive corners, like:

  • I, a user, set some inline config and run build(myConfig).
  • Hmm… build output looks like it used additional config I didn’t set 🤔
  • Why? Looks like it applied this on-disk config file 🤔
  • Why? I need to explicitly set configFile: false to disable that

I may be missing something though.

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
.changeset/many-pears-explode.md Outdated Show resolved Hide resolved
.changeset/many-pears-explode.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
@bluwy
Copy link
Member Author

bluwy commented Aug 14, 2023

Is it necessary to have the complexity of inline config plus file loading plus merging?

I’m trying to think of the scenarios and it seems you’d either use an on-disk config OR provide inline config, but not both. (In cases where you want some shared config which is overridden for a few values, I’d assume you can just import the shared config and spread it in?)

I actually copied the pattern from Vite (because that's what I'm used too and works well), but I find this most helpful for frameworks built on top of Astro (if there is one day). So those frameworks can provide their opinionated configs and also load the user config if needed.

I also think it's helpful in the scripting point of view. For example, I'm kicking off multiple Astro servers for testing, I want a base astro.config.mjs "template" to load, but I also want to dynamically pass more astro options for each tests, without needing to manually create a config file. (We need this for our tests currently)

Definitely agree with the unintuitive corners though as I've been bitten by it in Vite before, but I think it's worth it for the reasons above. I think it can be emphasize in the docs so users are more aware of it.

EDIT: Maybe I can see us making configFile slightly different by not loading the config files by default, unless if set to true or a string path?

@delucis
Copy link
Member

delucis commented Aug 14, 2023

Cool — OK, I’m happy you know what you’re doing. That all makes sense. (Although, for a tool built on top of Astro I might want user config to override some of my defaults, so that might still end up needing to eject out of our config loading potentially? Because currently the tool’s inline config would always take precedence over the user’s config file.)

Making the auto-loading opt-in with configFile: true is a possibility! I’ll let you decide if you think that’s best or not. It sounds it from my example above, but I could also see the argument that dev() should “just work” and behave the same way a standard astro dev run does.

'astro': major
---

Export experimental `dev`, `build`, `preview`, and `sync` APIs from `astro`. These APIs allow you to run Astro's commands programmatically, and replaces the previous entrypoint that runs the Astro CLI.
Copy link
Member

Choose a reason for hiding this comment

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

Just pointing out that I see the word "experimental" here and that typically means "behind an experimental flag" for us. If that's the case, then we should show the flag to enable and should include them in config-reference as an experimental entry.

If it's closer to something like "new" or "use at your own risk" or "good luck!", we might want to consider a different word or phrase that is closer to that meaning.

When it comes time for docs, we do have a CLI page in reference, and we could consider including this on that page. The page starts out pretty user-friendly, but then could progress into something like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The APIs are directly imported so I don't think we can have a flag for it. It's closer to "use at your own risk" though, and I'm not exactly sure what's the alternative to "experimental", maybe "work-in-progress" or "unstable"? 🤔

When it comes time for docs, we do have a CLI page in reference, and we could consider including this on that page. The page starts out pretty user-friendly, but then could progress into something like this.

That sounds great. Since these APIs are experimental, and it's signature will likely change, do you think it's worth introducing a full section for these APIs as a start? Or I was thinking we can lightly touch on the JS API names for each CLI commands:

astro dev
...
You can also run the command programatically with the experimental dev() API exported from the "astro" package.

Copy link
Member

Choose a reason for hiding this comment

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

Our usual approach with experimental things is to give them an isolated page or section — makes it easier to avoid lots of repetition saying each time something is mentioned is experimental, makes it easier to update the docs as experimental features change, plus avoids drawing too much attention to a feature we’re still stabilising.

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've made a first draft of the API under the CLI commands page as a section at withastro/docs#4234

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 that's a good place to start this, and keep it self-contained on that page as an Advanced section! Let's run with that idea for now.

@bluwy
Copy link
Member Author

bluwy commented Aug 15, 2023

Although, for a tool built on top of Astro I might want user config to override some of my defaults, so that might still end up needing to eject out of our config loading potentially? Because currently the tool’s inline config would always take precedence over the user’s config file.

@delucis I think that could be better solved indeed if/once we export a loadConfig or resolveConfig API, so they can load it themselves, and another mergeConfig API to merge their defaults. I'm leaning towards exposing those in later PRs though. Their implementations aren't quite public-ready yet.

For now they could also add an integration in the inline config to manually set fallback values if it's unset.

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.

I think this is fine, @bluwy , and I'll spend more time on the longer section that's going to be in docs!

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Went through the READMEs, changesets and comments, everything docs-wide LGTM!

@bluwy bluwy merged commit dbc97b1 into next Aug 18, 2023
17 checks passed
@bluwy bluwy deleted the export-js-api branch August 18, 2023 08:52
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.

None yet

7 participants