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

Allow custom const x = boolean exports to be read from plugins #8260

Closed
wants to merge 9 commits into from

Conversation

ottomated
Copy link
Contributor

@ottomated ottomated commented Aug 28, 2023

Changes

  • Any const boolean exports on a page can now be accessed on RouteData. This allows plugins to define custom per-page options.

Testing

  • Adds a new custom-page-options test

Docs

  • This should probably be added to plugin docs
    /cc @withastro/maintainers-docs for feedback!

@ottomated ottomated requested a review from a team as a code owner August 28, 2023 22:55
@changeset-bot
Copy link

changeset-bot bot commented Aug 28, 2023

🦋 Changeset detected

Latest commit: 2391b63

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 28, 2023
packages/astro/src/vite-plugin-scanner/scan.ts Outdated Show resolved Hide resolved
'astro': minor
---

Plugins can now access a page's boolean exports on RouteData['customOptions']
Copy link
Member

@ematipico ematipico Aug 29, 2023

Choose a reason for hiding this comment

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

I would call it externalOptions. It's more sound and communicates that these options don't come from Astro. Or pluginOptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should differentiate. If we accept this feature I think it should include all boolean options, including prerender.

The reason is, if this field is just for non-Astro defined booleans, that would mean that if Astro adds any new booleans itself, that becomes a breaking change. However if they are all under customOptions then there is no breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewp The only con to that approach is that we get a huge breaking change now that has a large impact on not only the astro codebase, but also breaks any plugins that rely on prerender. With the other approach, we might introduce breaking changes in the future, but any astro update that introduces a new page option will likely already be a major change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option here is that custom options must have some prefix we define. Similar to how HTML has the data- attributes. If they were required to be customFoo, for example, we could ensure that Astro never accidentally breaks an integration defined option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea but custom as a prefix feels a bit bulky to me

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.

I'm blocking this until we all get a chance to discuss as a team.

Really, really love this idea. I can think of four different problems this API could solve right off the bat—cache configuration, fragment/partial support, streaming control, and ISR.

That's very exciting, but it's also a good sign that we shouldn't rush to merge this without going through the roadmap process. We have a process in place precisely because PRs like this can have wide-ranging implications and we owe it to our users to think through all the consequences of them before merging.

@ottomated
Copy link
Contributor Author

I'm blocking this until we all get a chance to discuss as a team.

Sounds good! I don't have time to put together an RFC, so I'll back out and leave this code to be used as it can!

@natemoo-re
Copy link
Member

Sounds good! I don't have time to put together an RFC, so I'll back out and leave this code to be used as it can!

No worries at all, that's totally understandable! Hoping we can pick this up ASAP to explore for 3.1 or soon after.

@sevfurneaux
Copy link

sevfurneaux commented Sep 1, 2023

This is great @ottomated! @lilnasy pointed me to this PR, as it'll be a precursor to potential page specific adapters configuration (ISR, Vercel On-Demand ISR, Vercel Draft mode, Netlify Builders for singular pages) as outlined in this proposal.

One thing worth considering, is allowing for object properties to be exportable, and not just booleans. This would allow for more advanced configurations such as ISR expiry time and other adapter specific configurations as mentioned in the proposal:

export const config = {
  isr: {
    expiration: 60,
    bypassToken: BYPASS_TOKEN,
    allowQuery: ['search']
  }
};

Exportable objects rather than booleans would also allow for granular configuration for caching (as you mentioned @natemoo-re) in plugins, integrations and other adapters.

@matthewp
Copy link
Contributor

Still really like this idea. Hope it can get rolled into an RFC in the future. Closing as there's nothing actionable to be done right now.

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.

5 participants