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

Config : Airtable.s + Node for dynamic content importing #50

Closed
wants to merge 43 commits into from

Conversation

pkane
Copy link
Contributor

@pkane pkane commented Nov 28, 2023

Description

Add airtable.js dependency
Custom node script for pulling in airtable xd.gov content

Testing

For testing, make an edit to airtable-cache.json and commit to this branch.

@pkane pkane marked this pull request as draft November 28, 2023 17:49
@pkane pkane marked this pull request as ready for review December 8, 2023 20:21
Copy link
Member

@curt-mitch-census curt-mitch-census left a comment

Choose a reason for hiding this comment

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

LGTM! I left a bunch of comments on the airtable.js file, but they're mostly code style-related. The main thing to confirm or update is the Ruby version.

@@ -94,7 +96,7 @@ DEPENDENCIES
webrick

RUBY VERSION
ruby 3.1.3p185
ruby 3.2.2p53
Copy link
Member

Choose a reason for hiding this comment

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

Does the site still build with this version of Ruby? I ask because there was a Jekyll bug that caused us to fall back to 3.1.3. If it looks like everything is working as expected, we can go ahead and migrate to 3.2.2, in which case we should update the .ruby-version file as well since that's what cloud.gov uses when it builds the site (and we'll know if the version is still breaking with the Pages build Action step).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and actually I refrained from committing the .Gemfile with the version change of 3.1.3 -> 3.2.2.
Should I include that as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's go ahead and update both the Gemfile and .ruby-version.

_layouts/bios.html Outdated Show resolved Hide resolved
const fs = require('fs');
const Airtable = require('airtable');

const base = new Airtable({apiKey: process.env.AIRTABLE_API_KEY}).base('appuZMt69pZnTis2t');
Copy link
Member

Choose a reason for hiding this comment

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

Should we make the base ID an env variable as well?

airtable.js Outdated

// Utility function we'll use to compare our data
const deepCompare = (arg1, arg2) => {
if (Object.prototype.toString.call(arg1) === Object.prototype.toString.call(arg2)){
Copy link
Member

Choose a reason for hiding this comment

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

Instead of referencing call directly on the prototype's toString method, we should just be able to use toString directly:

Suggested change
if (Object.prototype.toString.call(arg1) === Object.prototype.toString.call(arg2)){
if (arg1.toString() === arg2.toString()){

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I pulled this from an old comparison snippet I had saved. I think it was written this way to avoid errors when trying to run .toString() on a null or undefined type. I might be mistaken and maybe this doesn't occur in ES6 anymore. But here's what I'm referring to...

To use the base Object.prototype.toString() with an object that has it overridden (or to invoke it on null or undefined), you need to call Function.prototype.call() or Function.prototype.apply() on it, passing the object you want to inspect as the first parameter (called thisArg).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/toString

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. In that case maybe it makes more sense to compare them via JSON.stringify:

Suggested change
if (Object.prototype.toString.call(arg1) === Object.prototype.toString.call(arg2)){
if (JSON.stringify(arg1) === JSON.stringify(arg2)){

airtable.js Outdated Show resolved Hide resolved
airtable.js Show resolved Hide resolved
airtable.js Show resolved Hide resolved
airtable.js Outdated Show resolved Hide resolved
airtable.js Outdated
// Utility function we'll use to compare our data
const deepCompare = (arg1, arg2) => {
if (Object.prototype.toString.call(arg1) === Object.prototype.toString.call(arg2)){
if (Object.prototype.toString.call(arg1) === '[object Object]' || Object.prototype.toString.call(arg1) === '[object Array]' ){
Copy link
Member

Choose a reason for hiding this comment

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

Here we can use the toString method directly for the first part of the conditional, and Array.isArray for the second part:

Suggested change
if (Object.prototype.toString.call(arg1) === '[object Object]' || Object.prototype.toString.call(arg1) === '[object Array]' ){
if (arg1.toString() === '[object Object]' || Array.isArray(arg1)){

.github/workflows/airtable.yml Outdated Show resolved Hide resolved
airtable.js Outdated
if (Object.prototype.toString.call(arg1) === Object.prototype.toString.call(arg2)){
if (Object.prototype.toString.call(arg1) === '[object Object]' || Object.prototype.toString.call(arg1) === '[object Array]' ){
if (JSON.stringify(arg1) === JSON.stringify(arg2)){
if (JSON.stringify(arg1) === '[object Object]' || Array.isArray(arg1)){
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should be more explicit for this line since the JSON.stringify method shouldn't ever return '[object Object]'. Here we should probably use typeof like so:

typeof arg1 === 'object'  || Array.isArray(arg1))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, sorry about that! I haphazardly grouped that in with the other changes.

@curt-mitch-census
Copy link
Member

Closing this PR in favor of #60 .

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.

3 participants