-
Notifications
You must be signed in to change notification settings - Fork 0
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
First pass at Core Data data loader #39
Conversation
✅ Deploy Preview for gbof-new-project ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for atlas-project-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gbof-c19nyc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gbof-new-project-2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pss-scavenger-hunt ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pan-african-data-project ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gbof-countermapping-new ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gbof-hudson-valley-new ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for gbof-hurricane-sandy-new ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get a PR up for the Astro upgrade? Also if we could rebase this on develop it'll make it much easier to see the changes specific to this task.
Two high level observations:
- We'll want to be able to deploy the site with or without the
static_pages
attribute in the config, the build shouldn't fail when it's omitted. We'll also probably want to renamestatic_pages
topages
, as they won't necessarily be static. - Using the
src/pages/[lang]/[model]/[slug].astro
page for testing is probably fine, but I don't think we'll want to include it in this PR for two reasons:- Models have different properties and will likely need dedicated pages. I don't think we'll be able to use a generic page for all model types.
- The model pages should also be available when not using content collections, which was why I was advocating for doing Create endpoints for fetching data #35 as the first task.
PR #19 has been up for a while. ^_^; I will rebase on develop, although I confess to confusion 'cause I thought I made this branch off of develop but perhaps I made it off a local version that was behind or something. In any case, yes, will do. (I guess I'll also rebase PR 19 while I'm at it.)
Agreed, this is what I've been working on this afternoon (and why I hadn't requested review yet here! 😅 ) -- I added a flag to the config that specifies whether the content collections should be created and whether the detail pages should be statically generated. (I've been fiddling with how to handle it a bit -- might need to be an env variable in order for Netlify to pick it up at the right point in the build process? It worked locally but in my testing on Netlify it's not detecting when the flag is set to
Hmm, fair enough, although I had understood this as the goal for now (pending specific projects having budget for per-model page designs). But I can factor those pages off into a separate branch for now.
This is also what I've been working on today -- I now have it set up so the detail pages will use SSR if the |
My mistaking, jumping the gun 🫤 . Carry on. |
…i18n default values
bce9183
to
de2f494
Compare
return fullResponse; | ||
} | ||
|
||
export function coreDataLoader(options: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on creating a loader per model type? We have some service classes in the @performant-software/core-data
package that can help with making the requests to the Core Data API so that we don't need to build the URLs and use the fetch
API.
We'll want loaders for the following model types:
- Events
- Instances
- Items
- Organizations
- People
- Places
- Works
Each loader would fetch the following relationships:
- Events
- Instances
- Items
- Manifests
- Media Content
- Organizations
- People
- Places
- Taxonomies
- Works
There will certainly be some duplication between loaders, but I think we can keep it DRY by using helper functions. The benefit here would that if we need to add any parameters or do any post-processing on a specific model type, it would be isolated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought a bunch about this and figured this would be one of your comments haha. I held off on this for the first pass because, as you say, it felt like so much duplication of functionality compared to just having one function to fetch relations for any model; but I take your point about the ability to do post-processing etc per model type. How are you picturing the helper functions working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have now factored out separate loaders per model type, but I ran into issues trying to import the services from react-components/core-data
-- when running the build, it was attempting to initialize some sort of i18next
thing and then exiting with a window is not defined
error:
i18next: languageChanged en
i18next: initialized {
debug: true,
initImmediate: true,
ns: [ 'translation' ],
defaultNS: [ 'translation' ],
fallbackLng: [ 'en' ],
fallbackNS: false,
whitelist: false,
nonExplicitWhitelist: false,
supportedLngs: false,
nonExplicitSupportedLngs: false,
load: 'all',
preload: false,
simplifyPluralSuffix: true,
keySeparator: '.',
nsSeparator: ':',
pluralSeparator: '_',
contextSeparator: '_',
partialBundledLanguages: false,
saveMissing: false,
updateMissing: false,
saveMissingTo: 'fallback',
saveMissingPlurals: true,
missingKeyHandler: false,
missingInterpolationHandler: false,
postProcess: false,
postProcessPassResolved: false,
returnNull: true,
returnEmptyString: true,
returnObjects: false,
joinArrays: false,
returnedObjectHandler: false,
parseMissingKeyHandler: false,
appendNamespaceToMissingKey: false,
appendNamespaceToCIMode: false,
overloadTranslationOptionHandler: [Function: overloadTranslationOptionHandler],
interpolation: { escapeValue: false },
lng: 'en',
resources: { en: { translation: [Object] } }
}
Deprecation warning: use moment.updateLocale(localeName, config) to change an existing locale. moment.defineLocale(localeName, config) should only be used for creating a new locale See http://momentjs.com/guides/#/warnings/define-locale/ for more info.
09:52:05 [ERROR] [content] window is not defined
So for now I kept the helper function as-is, but if you think there's a way to make the imported services work, I'm open to suggestions. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I think it has to do with the fact that the @performant-software/core-data
package (and dependencies) isn't exactly built to be used in a NodeJS environment. I think this is probably fine for now. In the future we should look into making that change to be able to use those service classes.
The reason I was hoping to use them here was because we'll need to do something similar in #35, and it would be much cleaner to not have to continuously build Core Data API URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These same helper functions can be used in #35 too, right? So it's still centralized to one place at least; if/when we figure out how to hook it up to the existing services.
src/content.config.ts
Outdated
|
||
let cols = {}; | ||
|
||
if (config.static_build) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make more sense as an environment variable? I've been using environment variables for more behind-the-scenes stuff and config.json
for more user-facing stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, yes; in fact I added just such an env variable at some point in order to try (and so far fail) to get Netlify to detect static vs SSR mode based on it. I can definitely switch this to look at that instead and take the flag out of the config.json
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is updated now.
package.json
Outdated
@@ -12,16 +12,16 @@ | |||
"@tailwindcss/typography": "^0.5.13", | |||
"@types/node": "^14.18.63", | |||
"@types/underscore": "^1.11.15", | |||
"astro": "^4.4.0", | |||
"astro": "^5.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge into develop
so that the Astro/Tina upgrade changes don't display here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, this update was merged into develop
.... PR #19 I am not sure why these are still showing up as changes....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I just merged the current version of dev into my branch and github is still showing these changes so I am again confused. 🤔
src/loaders/api/helpers.ts
Outdated
|
||
export async function fetchModelData( | ||
options: { | ||
projectId: number | number[] | string | string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ever using the projectId
anywhere? It looks like we're always grabbing it from the config, which is probably the safe thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic question. ^_^ I've updated it now so we're just grabbing them from the config file and not passing this as an options parameter.
In this PR
Relating to Issue #37, adds a data loader that pulls data about whitelisted models (listed in the config file) and stores it in a datastore in the Astro content layer.
Notes and questions
static_pages
attribute is added to the config file in the content repo.