implement "up-to-date" build design #1545
Conversation
|
Hello! This is an automated review by our custom reviewbot. It updates automatically when code or GitHub comments in this pull request are created or updated. Requested changesIf there are any common problems with the content files you created or modified, they will be listed here.
|
|
✔️ Deploy Preview for developer-chrome-com ready! 🔨 Explore the source changes: 3119d3b 🔍 Inspect the deploy log: https://app.netlify.com/sites/developer-chrome-com/deploys/615f666b98b30100081ece55 😎 Browse the preview: https://deploy-preview-1545--developer-chrome-com.netlify.app |
| const childProcess = require('child_process'); | ||
| const crypto = require('crypto'); | ||
|
|
||
| async function run() { |
There was a problem hiding this comment.
I don't think I see any await, couldn't this just be script.... Like, nothing in the run function, i.e. no run function?
There was a problem hiding this comment.
You're right, but this matches the other files, and it makes it easier if it ever needs to do async work. What do you think?
site/_data/tweets.js
Outdated
| // We have to cast this to unknown first as Typescript gets very literal about the underlying type | ||
| // of the JSON. | ||
| let tweets = /** @type {TwitterTweet[]} */ ( | ||
| /** @type {unknown} */ (require('../../external/data/tweets.json')) |
There was a problem hiding this comment.
Maybe we should use the cache tool here to point to the external tweets if there isn't a local one.
There was a problem hiding this comment.
I have mixed feelings on this but I'm happy to be disagreed with. I think there's a big advantage in not having to have conditionals here—the code always just pulls something in.
But I've added an eslint rule to allow this to be missing and that's awkward too.
WDYT?
|
I've updated the README and made the linter run.
|
|
No, we don't. The site needs to rebuild completely for a bunch of reasons,
one of the external data sources changing is fine.
…On Thu, 7 Oct 2021, 20:23 Ewa, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In external/build-external.js
<#1545 (comment)>
:
> + const options = {cwd: projectRoot, stdio: 'inherit'};
+
+ for (const script of scripts) {
+ const r = path.join(__dirname, script);
+ console.info('> Running', r);
+ try {
+ childProcess.execFileSync('node', [r], options);
+ } catch (e) {
+ // We don't log the error here, as we're already getting STDERR piped above.
+ console.warn(`! Failed to execute "${script}" (${e.status})`);
+ ++errors;
+ }
+ }
+
+ // Determine the hash for everything in data/.
+ const hash = crypto.createHash('sha256');
Do we consider partial updates to data? With hash including all files, we
would need to refetch/recompile all data (e.g. chrome api) even if Twitter
content changes?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1545 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA5DECEO66NDH74R4KP6CLUFVRH3ANCNFSM5FLBDY7A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Maybe, this is "normal" operation though where authors might run `npm dev`
lots. Should we log every time and say "External data up to date"?
…On Thu, 7 Oct 2021, 20:29 Ewa, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In external/maybe-sync-external.js
<#1545 (comment)>
:
> + '! Not synchronizing external data, previous local build found.' +
+ 'Run `npm run sync-external` to clear it.'
+ );
+ return;
+ }
+
+ let mtimeMs = 0;
+ try {
+ const stat = fs.statSync(path.join(__dirname, 'data'));
+ mtimeMs = stat.mtimeMs;
+ } catch (e) {
+ // The folder probably doesn't exist.
+ }
+ const since = +new Date() - mtimeMs;
+ if (since < syncThresholdMs) {
+ // Don't log at all, and don't synchronize.
Should we consol.log a warning on this occasion?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1545 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA5DEAU2OE7WX66LTFC4Z3UFVR7RANCNFSM5FLBDY7A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
external/build-external.js
Outdated
| // If this is a CI build, we start with everything found in "fallback/". It won't win, but it | ||
| // will be used in cases where credentials and such aren't available. | ||
| if (process.env.CI) { | ||
| const all = await syncFallback(); |
There was a problem hiding this comment.
Looks like /fallback data never gets refreshed and is used for testing only. Since its only used in CI, I'd call it e.g. testdata or so. "Fallback" suggests its data used when sync fails for some reason,
while it should never be used in prod.
This is a major PR changing the way we pull in external data to the site. It mostly implements the internal named design doc.
npm run dev, will have this folder synchronized to external/data/npm run build-external; this will also prevent sync until you later runnpm run sync-externalto clear a booleanThe idea is that external data breakages, while important (and will alert the team, because we alert on Cloud Build failures of any type), shouldn't stop regular deploy for our authors. They'll always get a "last known good" during local dev, or as part of our regular site deploy.
This only moves tweet generation to this approach. After this PR goes in, I'll add: