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

Use @sveltejs/adapter-static on GitHub Pages #5845

Closed
wants to merge 11 commits into from
Closed

Use @sveltejs/adapter-static on GitHub Pages #5845

wants to merge 11 commits into from

Conversation

NatoBoram
Copy link

@NatoBoram NatoBoram commented Aug 7, 2022

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Publishing on GitHub Pages requires writing special checks on environment variables in order to select adapter-static.

/** @returns {import('@sveltejs/kit').Adapter} */
function adapter() {
	if (process.env.GITHUB_ACTIONS) return adapterStatic({ fallback: '404.html' });
	return adapterAuto();
}

This could be automated by adapter-auto. To make this possible, actions/configure-pages was modified to accommodate SvelteKit.

This PR takes advantage of this recent modification to watch for $GITHUB_PAGES inside adapter-auto. By doing this, it becomes possible to publish to GitHub Pages from a GitHub Workflow without having to manually configure SvelteKit.

- uses: actions/configure-pages@v1
- name: Build
  run: pnpm run build
- uses: actions/upload-pages-artifact@v1
  with:
    path: build
- uses: actions/deploy-pages@v1

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 7, 2022

🦋 Changeset detected

Latest commit: 94d6594

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@sveltejs/adapter-auto Patch
@sveltejs/adapter-static Patch

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

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

This is great, thanks — comment in line, I think we can probably make the setup more self-explanatory

Comment on lines 62 to 72
SvelteKit can be automatically configured for deployment on GitHub Pages by using the `actions/configure-pages` GitHub Action before building it.

```yaml
- uses: actions/configure-pages@v1
- name: Build
run: pnpm run build
- uses: actions/upload-pages-artifact@v1
with:
path: build
- uses: actions/deploy-pages@v1
```
Copy link
Member

Choose a reason for hiding this comment

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

This could possibly use some more context — where do I put this code? Should we maybe have an example of a setting up a complete workflow, with instructions about where to write the .yml file etc?

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Sorry it took this long, was busy at work. I added more information about GitHub Workflows.

GitHub Pages

SvelteKit can be automatically configured for deployment on GitHub Pages by using the actions/configure-pages GitHub Action before building it. This action will set GITHUB_PAGES=true and replace kit.paths.base in svelte.config.js by the correct path for your GitHub repository.

To use it, create a new workflow file at .github/workflows/github-pages.yaml. You can start with GitHub's Node.js CI template, use the template provided here or make your own.

If you want to make your own workflow, it needs to perform these steps:

  1. Checkout the repository with actions/checkout
  2. Setup Node.JS with actions/setup-node
  3. Install dependencies with npm i, pnpm i or yarn
  4. Setup SvelteKit for GitHub Pages with actions/configure-pages
  5. Build the website with npm run build, pnpm run build or yarn run build
  6. Upload the built website with actions/upload-pages-artifact
  7. Deploy the uploaded website with actions/deploy-pages

Here's a complete template using pnpm.

name: GitHub Pages

on:
  push:
    branches: [main]

jobs:
  build:
    runs-on: ubuntu-latest

    strategy:
      matrix:
        node-version: [latest]

    steps:
      - uses: actions/checkout@v3

      - uses: pnpm/action-setup@v2
        with:
          version: latest

      - name: Use Node.js ${{ matrix.node-version }}
        uses: actions/setup-node@v3
        with:
          node-version: ${{ matrix.node-version }}
          cache: pnpm

      - name: Install dependencies
        run: pnpm install

      - uses: actions/configure-pages@v1

      - name: Build
        run: pnpm run build

      - uses: actions/upload-pages-artifact@v1
        with:
          path: build

      - uses: actions/deploy-pages@v1

@netlify
Copy link

netlify bot commented Sep 4, 2022

Deploy Preview for kit-demo ready!

Name Link
🔨 Latest commit 94d6594
🔍 Latest deploy log https://app.netlify.com/sites/kit-demo/deploys/631528b18b27440008ce5527
😎 Deploy Preview https://deploy-preview-5845--kit-demo.netlify.app/build
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

thank you! comments inline

@@ -56,6 +57,67 @@ You can also use `adapter-static` to generate single-page apps (SPAs) by specify

> You must ensure [`trailingSlash`](configuration#trailingslash) is set appropriately for your environment. If your host does not render `/a.html` upon receiving a request for `/a` then you will need to set `trailingSlash: 'always'` to create `/a/index.html` instead.

#### GitHub Pages
Copy link
Member

Choose a reason for hiding this comment

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

this section should really go in the adapter-static README (there's already a GitHub Pages section). maybe it should be an npm example instead of pnpm? many more people use it. or we could have something like

packages/adapter-static/examples/github-pages-npm/build.yaml
packages/adapter-static/examples/github-pages-pnpm/build.yaml

@@ -56,6 +57,67 @@ You can also use `adapter-static` to generate single-page apps (SPAs) by specify

> You must ensure [`trailingSlash`](configuration#trailingslash) is set appropriately for your environment. If your host does not render `/a.html` upon receiving a request for `/a` then you will need to set `trailingSlash: 'always'` to create `/a/index.html` instead.

#### GitHub Pages

SvelteKit can be automatically configured for deployment on GitHub Pages by using the [`actions/configure-pages`](https://github.com/actions/configure-pages) GitHub Action before building it. This action will set `GITHUB_PAGES=true` and replace `kit.paths.base` in `svelte.config.js` by the correct path for your GitHub repository.
Copy link
Member

Choose a reason for hiding this comment

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

the automatic kit.paths.base replacement makes me super nervous. hacks like this are guaranteed to break for a percentage of users, and it's hard to test locally. it should really be an environment variable that you add to your config — does something like this exist?

// svelte.config.js
export default {
  kit: {
    adapter: adapter(),
    paths: { base: process.env.GITHUB_PAGES_PATH ?? '' }
  }
};

Choose a reason for hiding this comment

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

To be clear, this is an optional feature of actions/configure-pages enabled with this setting. I think the reason they decided to go with config replacement is so that their starter-workflows would require zero config changes to work. I'm not entirely sure though...

In conversation with them about environment variables, they were also against providing environment variables for all the necessary site info (pages path, site origin, etc.). That's not to say the info isn't available. In a workflow, you could manually forward the data from their workflow step as outputs into environment variables in the build step.

In the adapter-static README, I think we could provide two workflows, with one using automatic replacement (warning users of potential issues) and the other using our own environment variables. The reason for this is that I don't think the automatic replacement version will go away. After this is merged, we hope to add a SvelteKit starter-workflow, which they may require to be using replacement (for zero config purposes, but I'm not sure yet).

We can definitely wait on the automatic replacement workflow until after we propose a starter workflow and see what they say.

defaults: () => ({
fallback: '404.html'
}),
done: (builder) => fs.writeFileSync(`${builder.config.kit.outDir}/.nojekyll`, '')

Choose a reason for hiding this comment

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

From what I can tell, the new custom GitHub Actions Pages deployments don't need a .nojekyll file like publishing from a branch does. So, I think this can be removed since we're going with the new deployment method.

@@ -56,6 +57,67 @@ You can also use `adapter-static` to generate single-page apps (SPAs) by specify

> You must ensure [`trailingSlash`](configuration#trailingslash) is set appropriately for your environment. If your host does not render `/a.html` upon receiving a request for `/a` then you will need to set `trailingSlash: 'always'` to create `/a/index.html` instead.

#### GitHub Pages

SvelteKit can be automatically configured for deployment on GitHub Pages by using the [`actions/configure-pages`](https://github.com/actions/configure-pages) GitHub Action before building it. This action will set `GITHUB_PAGES=true` and replace `kit.paths.base` in `svelte.config.js` by the correct path for your GitHub repository.

Choose a reason for hiding this comment

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

To be clear, this is an optional feature of actions/configure-pages enabled with this setting. I think the reason they decided to go with config replacement is so that their starter-workflows would require zero config changes to work. I'm not entirely sure though...

In conversation with them about environment variables, they were also against providing environment variables for all the necessary site info (pages path, site origin, etc.). That's not to say the info isn't available. In a workflow, you could manually forward the data from their workflow step as outputs into environment variables in the build step.

In the adapter-static README, I think we could provide two workflows, with one using automatic replacement (warning users of potential issues) and the other using our own environment variables. The reason for this is that I don't think the automatic replacement version will go away. After this is merged, we hope to add a SvelteKit starter-workflow, which they may require to be using replacement (for zero config purposes, but I'm not sure yet).

We can definitely wait on the automatic replacement workflow until after we propose a starter workflow and see what they say.

@Rich-Harris
Copy link
Member

The more I wrap the head round this, the more I think we shouldn't do it. The whole point of adapter-auto is that it's zero config, but this isn't — the developer needs to add a workflow, and the workflow needs to use the correct package manager etc. And actions/configure-pages injecting the base path by manipulating svelte.config.js is just a horrible idea. Sorry.

I would rather we simply add some instructions to the adapter-static README (or somewhere in that package) that shows people how to configure paths.base and set up a workflow that uses actions/upload-pages-artifact and actions/deploy-pages (but not actions/configure-pages, which AFAICT isn't doing anything we need). Does that make sense?

@Rich-Harris
Copy link
Member

Closing in favour of #6945 — thanks all

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

Successfully merging this pull request may close these issues.

5 participants