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

DatoCMS client library upgrade #58

Merged
merged 11 commits into from Dec 24, 2018
Merged

Conversation

DonatoM
Copy link
Collaborator

@DonatoM DonatoM commented Dec 24, 2018

No description provided.

@DonatoM DonatoM changed the title DatoCMs client library upgrade DatoCMS client library upgrade Dec 24, 2018
netlify.toml Outdated
@@ -5,3 +5,6 @@

[context.staging]
command = "npm run stage"

[context.donato_datocms_client_library_update]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove this once this PR gets approved.

Copy link
Owner

Choose a reason for hiding this comment

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

👍

title: this.title,
alt: this.alt,
url: this.url(),
path: this.path
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needed to override this method because components rely on receiving a relative URL rather than an absolute URL.

Copy link
Owner

Choose a reason for hiding this comment

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

should we refactor components to use the absolute URL in the future? If so, lets make a quick issue about it so we don't forget

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, that would remove the need for the monkey patch altogether. I'll go ahead and create an issue for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#59

Copy link
Owner

@elvingm elvingm left a comment

Choose a reason for hiding this comment

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

Leaving you the approval since I can trust you'll implement some things and do one final build on staging to verify everything is all good!

I think the routes generation is in routes-util already so that's the biggest thing I want to avoid moving back into static.config.js. But the focus here is just to get the site back up and working, we can revisit the rest next weekend.

netlify.toml Outdated
@@ -5,3 +5,6 @@

[context.staging]
command = "npm run stage"

[context.donato_datocms_client_library_update]
Copy link
Owner

Choose a reason for hiding this comment

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

👍

"datocms-search": "^0.1.7",
"dotenv": "^6.2.0",
Copy link
Owner

Choose a reason for hiding this comment

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

why do we need dotenv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was getting an error locally as well as on Netlify
screen shot 2018-12-24 at 4 19 20 pm

@@ -42,7 +42,7 @@ const HomePage = ({
</Head>
<div className="g-content-wrap">
<section className="featured-post">
<Post {...mainFeaturedPost} useThumbnail />
<Post {...mainFeaturedPost[0]} useThumbnail />
Copy link
Owner

Choose a reason for hiding this comment

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

is this because of the other logic that turns non-arrays into an array with a single item? Or is this something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, it's the logic that turns everything into an array.


export default class DatoClient {
constructor(credentials = { token: null }) {
if (!credentials.token) throw new Error('Missing Datocms Credentials');
this.client = new SiteClient(credentials.token);
this.loader = new Loader(this.client, false);
Copy link
Owner

Choose a reason for hiding this comment

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

I think the second argument of the constructor defaults to false already so we can remove that from here i think. ref

return this.client.fields.find(itemTypeID);
await this.loader.load();
const content = this.loader.itemsRepo.collectionsByType;
return content;
Copy link
Owner

Choose a reason for hiding this comment

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

we can just return here since we're not using the variable.

Copy link
Owner

Choose a reason for hiding this comment

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

in the future maybe we wrap the itemsRepo properties in getter methods here for convenience. Nice shit nonetheless, man!

});

let fields = [];
const uniqueFields = uniq(models.reduce((arr, m) => arr.concat(m.fields), []));
Copy link
Owner

Choose a reason for hiding this comment

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

thank god we deleted this.

return item;
});
modelContent.forEach(record => {
try {
Copy link
Owner

Choose a reason for hiding this comment

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

why are we wrapping this in a try/catch if we don't throw any errors at all or use any Promises here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sometimes calling toMap() raises an error. Didn't have much time to investigate. Nothing stuck out whem checking pages in staging. Perhaps this is something we can come back to.

Stacktrace for the error I get when calling toMap():

TypeError: Cannot read property 'width' of undefined
[0]     at File.get (/Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/local/fields/File.js:75:26)
[0]     at /Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/utils/seoTagsBuilder.js:133:20
[0]     at Array.find (native)
[0]     at image (/Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/utils/seoTagsBuilder.js:132:8)
[0]     at /Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/utils/seoTagsBuilder.js:149:18
[0]     at Array.reduce (<anonymous>)
[0]     at seoTagsBuilder (/Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/utils/seoTagsBuilder.js:148:34)
[0]     at Item.get (/Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/local/Item.js:235:44)
[0]     at Item.toMap (/Users/donatocruz/Desktop/jc/jewish-currents-static/node_modules/datocms-client/lib/local/Item.js:105:16)
[0]     at /Users/donatocruz/Desktop/jc/jewish-currents-static/src/datocms/normalizer.js:34:45

static.config.js Outdated
const final = flatten(routes);
console.log(`Processed ${final.length} Post routes`);
return final;
};
Copy link
Owner

Choose a reason for hiding this comment

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

Why did we move these pagination functions back in here? Can't we keep them where they were or were there changes made to them?

I want to keep this file from growing out of proportion with what is necessary for understanding React Static's main config file. As a sidenote, we could also make an issue for changing this all up to utilize React Static's logic for generating page routes: makePageRoutes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad I'm not sure why I copied this a while ago. Created the issue here: #60

@DonatoM DonatoM merged commit 9f11679 into staging Dec 24, 2018
@DonatoM DonatoM deleted the donato_datocms_client_library_update branch December 24, 2018 21:55
DonatoM added a commit that referenced this pull request Dec 24, 2018
* upgrade datocms-clientjs library, adjust data normalizer to properly search for the model type

* update the netlify staging environment branch

* add dotenv dependency

* add build wip refactor

* monkey patch file toMap implementation and remove unused code

* leave a comment around required fields mapping and refactor build process a bit

* leverage local build cache

* remove unnecessary undefined checks

* remove duplicate code

* remove explicit declaration of previewMode in the Dato Loader

* no longer automatically deploy this branch to the staging environment
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.

None yet

2 participants