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

[WIP] Add contentful client with support for local static data #7

Closed
wants to merge 2 commits into from

Conversation

davidbasalla
Copy link

@davidbasalla davidbasalla commented Jan 22, 2018

It's useful to have some static data for the 'Events' content type to play around with. This PR aims to provide a recorded API response from a static JSON file, via an API wrapper around the contentful client. The data gets rendered via a Flatlist, see screenshot below...

The wrapper for the contentful client does a couple of things:

  • provides an allEvents() method that returns all events
  • returns only the .fields values as presumably we only care about those (see 'data.json')
  • provides a mock client which returns static data for the 'Event' content type from a JSON file ('data.json', derived from a real response for a toy 'Event' content type).

To make a real request against the API, the env vars SPACE and ACCESS_TOKEN will need to be set, and the ContentfulClient instance will have to be created with fetchStaticData=false.

Slightly inspired by this discussion about adding caching to the contentful client, which we could investigate at a later point.

Still need to add tests!!

Screenshot

screen shot 2018-01-22 at 17 39 04

Pre-flight check-list

Before raising a pull request

  • Documentation
  • Unit tests
  • Build passing

Pre-merge check-list

  • Code review (At least one 👍)
  • Tester approved

@davidbasalla
Copy link
Author

I seem to get errors with the android build step fairly often. After retrying the job it seems to (sometimes) go green...

[17:37:19]: ▸ Scanning folders for symlinks in /home/circleci/project/node_modules (21ms)
[17:37:19]: ▸ Scanning folders for symlinks in /home/circleci/project/node_modules (23ms)
[17:37:21]: ▸ Loading dependency graph, done.
[17:37:21]: ▸ warning: the transform cache was reset.
[17:37:39]: ▸ :app:bundleReleaseJsAndAssets FAILED
[17:37:39]: ▸ FAILURE: Build failed with an exception.
[17:37:39]: ▸ * What went wrong:
[17:37:39]: ▸ Execution failed for task ':app:bundleReleaseJsAndAssets'.
[17:37:39]: ▸ > Process 'command 'node'' finished with non-zero exit value 137
[17:37:39]: ▸ * Try:
[17:37:39]: ▸ Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
[17:37:39]: ▸ BUILD FAILED
[17:37:39]: ▸ Total time: 1 mins 19.916 secs

import { Platform, StyleSheet, Text, View } from "react-native";

const instructions = Platform.select({
ios: "Press Cmd+R to reload \n Cmd+D or shake for dev menu",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind whacking these instructions into the readme? Might be handy to remember how to get the dev menu again!

Copy link
Author

Choose a reason for hiding this comment

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

Have added this to the README and also removed all Expo references...

@robbiemccorkell
Copy link
Contributor

I only just noticed after smashing a load of review comments on that this PR was [WIP]!

Feel free to ignore me if you're still working on it.

@davidbasalla
Copy link
Author

@robbiemccorkell please feel free to comment, any feedback is very much welcome!

@davidbasalla davidbasalla force-pushed the add-contentful-client branch from 8d789bf to 3905d85 Compare January 23, 2018 09:59
It's useful to have some static data for the 'Events' content type to play around with. This PR aims to provide a recorded API response from a static JSON file, via an API wrapper around the `contentful` client.

The wrapper for the [`contentful` client](https://github.com/contentful/contentful.js) does a couple of things:
- provides an `allEvents()` method that returns all events
- returns only the `.fields` values as presumably we only care about those (see 'data.json')
- provides a mock client which returns static data for the 'Event' content type from a JSON file ('data.json', derived from a real response for a toy 'Event' content type).

To make a real request against the API, the env vars `SPACE` and `ACCESS_TOKEN` will need to be set, and the `ContentfulClient` instance will have to be created with `fetchStaticData=false`.
We've decided against using Expo so this commit removes all references to it.
@davidbasalla davidbasalla force-pushed the add-contentful-client branch from 3905d85 to 0402c90 Compare January 23, 2018 10:00
@davidbasalla
Copy link
Author

Thinking about this some more, this approach probably won't scale well... retrieving JSON data from Contentful and storing it here as we continue to create new content types and actual content will become quite tedious, with the risk of us not updating the local data and it becoming stale.

Maybe some sort of local cache for API requests would be better (eg https://github.com/ptarjan/node-cache)... that way we could still reduce the number of calls to Contentful during development. @robbiemccorkell have you used anything to cache API requests on previous projects? Do you think it's even worth doing at this point? My initial motivation for opening this PR was to limit the requests to Contentful but not sure if that's a big concern... (ignoring the fact that we don't have any access yet to Contentful)

@robbiemccorkell
Copy link
Contributor

@davidbasalla Oh wait you don't have access to contentful? You should. I invited you on your badger email. Or do you mean access to their old account? And actually Pride have managed to get their plan switched over so we have a proper paid plan now.

I agree, now that we're on a proper plan I don't think the mock data will really be needed. As for number of requests it looks like the plan we've been given will allow 6M requests per month. So I think we should be ok!

I think we could totally look at caching at some point, but we should probably only do so when it becomes a problem so we're not optimising too early.

@davidbasalla
Copy link
Author

Sounds good, I'll mothball this PR for now then since it doesn't add much value... In the meantime I'll focus on setting up some data in Contentful (in a staging environment). And sorry yes I do have access, I think I was expecting to see some of the old data but I guess it makes sense to start from scratch.

I might revive part of this for when we need to stub API responses in tests.

@robbiemccorkell
Copy link
Contributor

Ok if you like.

Yea it looks like we won't be able to export the full data out of the old contentful. I just copied and pasted a few example entries for testing.

I think there's definitely still value in having the contentful integration in a separate file with some useful functions for accessing. I guess this will spring up in a future PR.

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.

2 participants