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

Replace hardcoded keys with the centralized list of Airtable keys #153

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

Conversation

sea-witch
Copy link
Collaborator

Resolves #124

@@ -1,6 +1,6 @@
const { merge } = require("lodash");
const _ = require("lodash");
const { airbase } = require("~airtable/bases");
const { airbase } = require("../bases");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason I had to make this a relative path. The tilde path threw an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

very weird! do you remember the error? this should be currently in prod right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, super weird! The version with the tilde path is in prod, which proves that it works. Maybe this is just user error on my part.

Here's the error: Module not found: Can't resolve '~airtable/bases' in '/Users/emmasmith/projects/mutual-aid-app/src/lib/airtable/tables'

I see this when I run npm run local:react. Also when I try to build for prod using npm run build. The Express app runs just fine. It seems that the React side of things is not respecting the moduleAliases.

Clues, advice, guidance most appreciated :)

@@ -12,6 +12,7 @@ import HouseholdSizeChip from "./HouseholdSizeChip";
import DrivingClusterChip from "./DrivingClusterChip";
import { daysSinceSlackMessage } from "../helpers/time";
import ClusterMapContext from "../context/ClusterMapContext";
import { fields } from "../../lib/airtable/tables/requests";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same here: the absolute path (with or without tilde) didn't work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly this is old news, but after looking over it, it looks like the ~whatever module aliases aren't in use anywhere in the webapp directory... surprising! But I think that at least means that this problem isn't because of any change in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it looks like most of the other scripts in our package.json are requiring module-alias/register which I assume is what's giving other scripts the ability to resolve those aliases. However, I'm not actually sure how to do that in local:react since it runs react-scripts instead of node.

Copy link
Collaborator Author

@sea-witch sea-witch left a comment

Choose a reason for hiding this comment

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

There are a few raw airtable strings in updateMessageContent.js, but they don't show up in the airtable fields translation object. Should they be added?

@sea-witch sea-witch requested a review from mjmaurer September 28, 2020 17:51
@mjmaurer
Copy link
Contributor

There are a few raw airtable strings in updateMessageContent.js, but they don't show up in the airtable fields translation object. Should they be added?

yes please!

@sea-witch sea-witch mentioned this pull request Sep 30, 2020
Base automatically changed from master to main February 10, 2021 00:16
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.

Use airtable request fields in place of hardcoded keys
3 participants