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

meta: add snapshot builds for pr previews #1525

Open
wants to merge 1 commit into
base: master
from

Conversation

@devsnek
Copy link
Member

commented Apr 30, 2019

this commit adds a warning (stolen from whatpr.org) to "snapshot" builds that exist for non-reference reasons, such as pr previews.

if we add some sort of github action to auto deploy netlify or something on a new pr and then post a comment with the url, this will provide a nice preview. someone with permissions on this repo will have to set that part up.

rendered: https://5ce61b1a493e120007e63323--brave-elion-abb3d4.netlify.com/

@devsnek devsnek force-pushed the devsnek:snapshots branch from 6afc10d to 863d3df Apr 30, 2019

Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated
Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated
Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated
Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated
Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated
Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated

console.log('Inserting snapshot reference warning...');

const dom = new JSDOM(fs.readFileSync('./out/index.html', 'utf8'));

This comment has been minimized.

Copy link
@domenic

domenic Apr 30, 2019

Member

Don't convert to string; pass the buffer directly. See https://github.com/jsdom/jsdom#encoding-sniffing last paragraph

Show resolved Hide resolved scripts/insert_snapshot_warning.js Outdated

@devsnek devsnek force-pushed the devsnek:snapshots branch from 863d3df to 1e9eadf Apr 30, 2019

@ljharb ljharb added the meta label Apr 30, 2019

@devsnek devsnek force-pushed the devsnek:snapshots branch 2 times, most recently from 2e9a867 to 8c016c4 May 23, 2019

@devsnek

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

i managed to make this automatically apply the snapshot thing if netlify is building a non-production build. if this was set up for this repo, it believe it would automatically post this link when the pr was created.

preview: https://5ce61b1a493e120007e63323--brave-elion-abb3d4.netlify.com/ vs https://brave-elion-abb3d4.netlify.com/

settings i used:

</details>
`;

const WARNING_CSS = `

This comment has been minimized.

Copy link
@ljharb

ljharb May 23, 2019

Member

could we store this in a .css file instead of inlining it?

This comment has been minimized.

Copy link
@devsnek

devsnek May 23, 2019

Author Member

i think its better encapsulated as inlined, any reason to move it to a separate file?

This comment has been minimized.

Copy link
@ljharb

ljharb May 23, 2019

Member

syntax highlighting, linting, reuse, etc

@@ -10,6 +10,7 @@
"build": "npm run build-master",
"build-for-pdf": "npm run build-master -- --old-toc",
"build-travis": "npm run build-master && npm run build-es2019",
"build-snapshot": "npm run clean && npm run build-master && node scripts/insert_snapshot_warning.js",

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member
Suggested change
"build-snapshot": "npm run clean && npm run build-master && node scripts/insert_snapshot_warning.js",
"prebuild-snapshot": "npm run clean",
"build-snapshot": "npm run build-master && node scripts/insert_snapshot_warning",
const { JSDOM } = require('jsdom');
const { execSync } = require('child_process');

const COMMIT = execSync('git rev-parse --verify HEAD').toString();

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member
Suggested change
const COMMIT = execSync('git rev-parse --verify HEAD').toString();
const COMMIT = String(execSync('git rev-parse --verify HEAD'));

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 6, 2019

Author Member

any particular reason for this?

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member

oh this is just a nit; mainly that calling .toString doesn’t ensure you get a string out; only String() or interpolating into a template literal both ensures you call toString and coerce to a string - so i have a general code review policy of “never call toString with no args directly”.

set -ex

npm run clean
npm run build-master

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member

is this file something netlify just knows to look for, as opposed to being able to wire up an npm script?

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 6, 2019

Author Member

netlify can run any command, but i don't think i can inline the $CONTEXT check in the package.json

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member

you can, with some awkward bash, but also perhaps the check could go in the script itself?

npm run build-master

if [ "$CONTEXT" != "production" ]; then
node scripts/insert_snapshot_warning.js

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 6, 2019

Member
Suggested change
node scripts/insert_snapshot_warning.js
node scripts/insert_snapshot_warning

@devsnek devsnek force-pushed the devsnek:snapshots branch from d49b7df to 0f2e45e Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.