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

Add SvelteKit as an option for static_site_generator setting #16

Merged
merged 7 commits into from
Aug 17, 2022

Conversation

AndrewLester
Copy link
Contributor

@AndrewLester AndrewLester commented Aug 10, 2022

Add SvelteKit as an option for use as a static site generator.

I had set up the config parsing changes earlier today, but I added the actual SvelteKit config code more recently based on @NatoBoram's snippet. In the snippet, he provided an example for setting a GITHUB_PAGES environment variable, which has been discussed in the same thread. I didn't include this just yet, as I might suggest moving it outside of the SvelteKit SSG option into a more global spot, since it may not be SvelteKit specific. This all depends on the outcome of the thread, though.

@AndrewLester AndrewLester requested a review from a team as a code owner August 10, 2022 00:37
@NatoBoram
Copy link
Contributor

an example for setting a $GITHUB_PAGES environment variable, which has been discussed in actions/upload-pages-artifact#9. I didn't include this just yet, as I might suggest moving it outside of the SvelteKit SSG option into a more global spot, since it may not be SvelteKit specific

I doubt that $VERCEL, $CF_PAGES and $NETLIFY are SvelteKit-specific, but those are still the ones used by SvelteKit here, so I think it's okay. After all, if something else relies on $GITHUB_PAGES for whatever reason, it probably also wants it to be true in the context where this action is used anyway

@JamesMGreene
Copy link
Contributor

Since SvelteKit config files use ExportDefaultDeclaration with an identifier, I had to modify the findConfigurationObject method to support this. It just uses the existing indirect module export code essentially.

This is actually something I had added in #15 (which is now merged) as well. 😊 👍🏻

Unfortunately, that means some merge conflicts here. 😅

@AndrewLester
Copy link
Contributor Author

This is actually something I had added in #15 (which is now merged) as well. 😊 👍🏻

Unfortunately, that means some merge conflicts here. 😅

No worries, I just rebased on those changes. I also did add the GITHUB_PAGES env variable export to index.js, as I think that's where it could live depending on our thoughts in this thread.

@NatoBoram
Copy link
Contributor

I just re-read some comments and I noticed that @yoannchaudet considers $GITHUB_PAGES and actions/configure-pages's own outputs to

pollute the environment (with Svelte specific environment variables)

Maybe we shouldn't put it in index.js after all? Let's put it back in src/set-pages-path.js and hope that someone merges it quickly so we can continue sveltejs/kit#5845

@AndrewLester
Copy link
Contributor Author

AndrewLester commented Aug 14, 2022

I see what you mean about his comment, but in my opinion exporting such a general variable in SvelteKit only configuration is a brittle solution that breaks ownership principles.

I see two specific flaws with it. First, people may end up wanting to use the variable in other configurations since it's so general, and it has to be moved (or people may forget about it). Second, the method it would be placed in should only handle configuration setup, and giving it another task of setting the environment variable would be confusing.

I do agree that making this sveltekit only could have it get merged faster, but it's hard to tell because I don't think we've really done any convincing. I just gave you collaborator access to my repo if you want to move it, but I'd suggest moving it to a separate, environment variable setup function similar to config setup that checks for SvelteKit.

src/index.js Outdated
@@ -18,6 +18,7 @@ async function main() {
setPagesPath({ staticSiteGenerator, generatorConfigFile, path: siteUrl.pathname })
}
outputPagesBaseUrl(siteUrl)
core.exportVariable('GITHUB_PAGES', true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We had the discussion with the team and we are fine having a global environment variable exposed here.

Copy link
Collaborator

@yoannchaudet yoannchaudet left a comment

Choose a reason for hiding this comment

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

Looking good to me. @JamesMGreene opinion :)?

Copy link
Contributor

@JamesMGreene JamesMGreene left a comment

Choose a reason for hiding this comment

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

Looks good to me, minus one nit about the data type of the environment variable export. 👍🏻

src/index.js Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
@JamesMGreene JamesMGreene merged commit f71d3d0 into actions:main Aug 17, 2022
@JamesMGreene
Copy link
Contributor

This is now released in v1.2.0! 🚀

You can use it with any of the following uses: statements (from most secure/specific to least):

uses: actions/configure-pages@f71d3d08f0abfe9cbb23e527dc9f633beabf97bc
uses: actions/configure-pages@v1.2.0
uses: actions/configure-pages@v1
uses: actions/configure-pages@main

Thank you for the contribution! 💝

@AndrewLester AndrewLester deleted the ssg-sveltekit branch August 18, 2022 04:19
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