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

[actions] Publish PR previews #38

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gibson042
Copy link

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?

.github/workflows/publish-main.yml Outdated Show resolved Hide resolved
Comment on lines +40 to +41
const listArtifactsResponse =
await github.rest.actions.listWorkflowRunArtifacts(listArtifactsQuery);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const listArtifactsResponse =
await github.rest.actions.listWorkflowRunArtifacts(listArtifactsQuery);
const listArtifactsResponse = await github.rest.actions.listWorkflowRunArtifacts(listArtifactsQuery);

Comment on lines +44 to +47
const summary = artifacts?.map(artifact => {
const { name, size_in_bytes, url } = artifact;
return { name, size_in_bytes, url };
});
Copy link
Member

Choose a reason for hiding this comment

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

maybe?

Suggested change
const summary = artifacts?.map(artifact => {
const { name, size_in_bytes, url } = artifact;
return { name, size_in_bytes, url };
});
const summary = artifacts?.map(({ name, size_in_bytes, url }) => ({ name, size_in_bytes, url }));

.github/workflows/publish-pr.yml Outdated Show resolved Hide resolved
Comment on lines +38 to +39
const listArtifactsResponse =
await github.rest.actions.listWorkflowRunArtifacts({ owner, repo, run_id });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const listArtifactsResponse =
await github.rest.actions.listWorkflowRunArtifacts({ owner, repo, run_id });
const listArtifactsResponse = await github.rest.actions.listWorkflowRunArtifacts({ owner, repo, run_id });

Comment on lines +34 to +36
formatErrors.push(SyntaxError(`bad placeholder at index ${i}: ${trunc}`));
} else if (!Object.hasOwn(data, name)) {
formatErrors.push(ReferenceError(`no data for ${m}`));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
formatErrors.push(SyntaxError(`bad placeholder at index ${i}: ${trunc}`));
} else if (!Object.hasOwn(data, name)) {
formatErrors.push(ReferenceError(`no data for ${m}`));
formatErrors.push(new SyntaxError(`bad placeholder at index ${i}: ${trunc}`));
} else if (!Object.hasOwn(data, name)) {
formatErrors.push(new ReferenceError(`no data for ${m}`));

}
return data[name];
});
if (formatErrors.length > 0) throw AggregateError(formatErrors);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (formatErrors.length > 0) throw AggregateError(formatErrors);
if (formatErrors.length > 0) throw new AggregateError(formatErrors);

Comment on lines +67 to +71
const stayInHead = new Set([
...['base', 'basefont', 'bgsound', 'link', 'meta', 'title'],
...['noscript', 'noframes', 'style', 'script', 'template'],
'head',
]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const stayInHead = new Set([
...['base', 'basefont', 'bgsound', 'link', 'meta', 'title'],
...['noscript', 'noframes', 'style', 'script', 'template'],
'head',
]);
const stayInHead = new Set([].concat(
['base', 'basefont', 'bgsound', 'link', 'meta', 'title'],
['noscript', 'noframes', 'style', 'script', 'template'],
'head',
));

const results = await Promise.allSettled(work);

const failures = results.filter(result => result.status !== 'fulfilled');
if (failures.length > 0) throw AggregateError(failures.map(r => r.reason));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (failures.length > 0) throw AggregateError(failures.map(r => r.reason));
if (failures.length > 0) throw new AggregateError(failures.map(r => r.reason));

@gibson042
Copy link
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 :-)

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
2 participants