Skip to content

Conversation

@gibson042
Copy link
Member

Fixes #30

  • Rename .github/workflows/deploy.yml → .github/workflows/publish-main.yml.
  • Replace .github/workflows/build.yml with .github/workflows/render-pr.yml, which uploads the result as an artifact.
  • Introduce .github/workflows/publish-pr.yml, which runs on completion of render-pr.yml and publishes into gh-pages at pr/$PR (and adds a comment with that link).
  • publish-pr.yml also adds a preview warning with code derived from ecma262 insert_snapshot_warning.js (see example at https://tc39.es/proposal-immutable-arraybuffer/pr/21/ ).
  • Once this settles, I'll open a PR to align ecma262 preview warnings as well.
Preview Warning

collapsed

expanded

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

isn't there a risk here that anyone can get any content deployed under tc39.es just by making a PR?

@gibson042
Copy link
Member Author

isn't there a risk here that anyone can get any content deployed under tc39.es just by making a PR?

In a sense, yes—and AFAICT, fundamental to the request of #30. This implementation publishes to https://tc39.es/$repo_name/pr/$digits the result of any successful npm run build from a PR branch, with *.html files modified to include the contents of pr_preview_warning.html from the main branch. Individual proposals already can and do provide this functionality, but there's obvious value to adoption here in this template so instances automatically inherit it.

Are there missing safeguards that you'd like? And if so, can you describe them?

As for the style suggestions, I don't feel strongly but would point out that the line breaks were inserted by Prettier with default settings.

@ljharb
Copy link
Member

ljharb commented Jan 10, 2025

yes, prettier often produces ugly code, unfortunately. (a line break should never ever ever appear after a =, for example)

re the safeguards, i'm not sure if we even need any, i just wanted to bring it up :-)

@bakkot
Copy link
Member

bakkot commented Apr 29, 2025

@gibson042 Is there a reason this uses a lot of JS to download the artifact for publishing instead of
actions/download-artifact?

@gibson042
Copy link
Member Author

@gibson042 Is there a reason this uses a lot of JS to download the artifact for publishing instead of actions/download-artifact?

Yes, in fact there are multiple reasons:

  1. Better output to assist debugging if there is ever a problem.
  2. Avoiding the need to supply an explicit token (cf. Download Artifacts from other Workflow Runs or Repositories: "By default, the permissions are scoped so they can only download Artifacts within the current workflow run. To elevate permissions for this scenario, you can specify a github-token along with other repository and run identifiers").

And the JS only looks like "a lot" for the first point (and to a lesser extent from Prettier):

const { listWorkflowRunArtifacts, downloadArtifact } = github.rest.actions;

const { owner, repo } = context.repo;
const run_id = ${{ github.event.workflow_run.id }};
const name = process.env.ARTIFACT_NAME;
const listArtifactsParams = { owner, repo, run_id, name };
const listArtifactsResponse = await listWorkflowRunArtifacts(listArtifactsParams);
const { total_count, artifacts } = listArtifactsResponse.data;
// …error handling…
const artifact_id = artifacts[0].id;

console.log(`downloading artifact ${artifact_id}`);
const downloadParams = { owner, repo, artifact_id, archive_format: 'zip' };
const downloadResponse = await downloadArtifact(downloadParams);
const fs = require('fs');
fs.writeFileSync('${{ github.workspace }}/result.zip', Buffer.from(downloadResponse.data));

@gibson042 gibson042 requested a review from ljharb June 2, 2025 17:57
@ljharb
Copy link
Member

ljharb commented Jun 2, 2025

@gibson042 would this technique perhaps fix previews on the main spec?

@gibson042
Copy link
Member Author

Yes.

Once this settles, I'll open a PR to align ecma262 preview warnings as well.

@ljharb ljharb force-pushed the gh-30-pr-preview branch from eb76c74 to 2983ef4 Compare June 2, 2025 19:34
@ljharb ljharb merged commit 2983ef4 into tc39:main Jun 2, 2025
1 check passed
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.

Suggestion: PR Preview Github Action Workflows

3 participants