Adds support for global key #42

ninjasort · 2016-11-18T00:23:33Z

#36 With this PR, users can now add entry_key and entry_extension to their global settings and transform entries from Contentful into files within Metalsmith. The files are created from the entry_key property and must be referenced properly to the path of the file. The file with have the entry_extension as the extension to the file. The files will add contents as a Buffer for other Metalsmith plugins.

Example:

... // global settings
entry_key: '_key'
entry_extension: 'md'

An entry in Contentful with properties:

_key: pages/index
contents: '# My Home Page'

Output:

build/pages/index.html => <h1>My Home Page</h1>

… ref contentful#36

stefanjudis · 2016-11-18T13:31:22Z

@cameronroe Hey hey, thanks so much for bringing this in. :)

Test are failing currently?

I'll have a deeper look into it on monday. :)

stefanjudis

Just had a quick look. :)

stefanjudis · 2016-11-18T13:36:04Z

lib/processor.js

@@ -139,6 +143,29 @@ function processEntriesForFile (file, entries, options) {
  return files
 }

+function filterEntries (entries, file, options) {


JSDocs are missing.

stefanjudis · 2016-11-21T09:17:24Z

@cameronroe Still failing? Do you need help with this?

ninjasort · 2016-11-21T10:09:18Z

@stefanjudis Looks like a directory wasn't pushed to git, then .gitkeep broke the tests. Just fixed. Should be good for a review. 👍

stefanjudis

Okay I looked over the code and it looks good. The only thing I'm worried about is documentation...

It's becoming really complex and I think we should improve that now or in another PR.

For this PR what would be great would be a reworked getting started guide maybe also including a few screenshots on how to connect contentful with metalsmith?

What do you think? We can do this in a seperate PR, if you like, but I wouldn't release it without solid help, docs and guidance.

stefanjudis · 2016-11-21T10:55:03Z

docs/global-settings.md

+### `entry_extension` *(optional)*
+
+If you specify `entry_key`, you will need to specify the entry extension for all file keys. This will be appended to the key on Contentful entries that contain the `entry_key`.
+
 ### `filterTransforms` *(optional)*


I feel like we need a lot more documentation now... And maybe update the getting started guide.
This addition makes total sense and is probably the way to go, but it's not easy to understand in the beginning.

Right, do you think there should be another section similar to this one? I could write up a longer description under this section and then link it to the docs/global-settings.md?

stefanjudis · 2016-11-21T10:57:53Z

README.md

+      "space_id": "YOUR_CONTENTFUL_SPACE_ID",
+      "entry_key": "_key",
+      "entry_extension": "md",
+      "contentful": {


~~Is the contentful property correct here?~~~

Found it.

stefanjudis · 2016-11-21T10:58:54Z

docs/global-settings.md

@@ -82,6 +97,8 @@ layout: posts.html
 ---
 ```

+### [`contentful` *(optional)*](source-file-settings.md)


This needs definitely more explanation. :)

ninjasort · 2016-11-22T01:58:02Z

Sure, that's cool. What were you thinking for the getting started guide? We can do that in this PR.

stefanjudis · 2016-11-22T09:22:26Z

@cameronroe I feel like we should rework the getting starting guide, as this implementation feels like the right way to go. So I'd recommend it for every new user (not breaking the old functionality yet).

IMO the documentation of contentful-metalsmith is definitely the weakest part right now.

So in my head it's kind of like this (including screenshots of contentful.

## Getting started

- Set these global configs
- head over to contentful 
  - set the properties you wish
  - ...
  - ...
- and ready!

### Way to still use base source files

... Maybe linking to a seperated readme to not bloat the main readme ...

## Global options

Quick explanation
... linking to another md ...

## Source file options

Quick explanation
... linking to another md ...

And just an idea that comes to mind right now... I would be cool when we could provide the base structure into contentful and explain it in the getting started section. :)

Great that we can do this in this PR. 👍

Does all this make sense?

ninjasort · 2016-11-23T05:02:08Z

I updated the readme with all the correct configs to get started. I'm not so sure exactly what you're needing in the other sections. Everything looks like it's already detailed. Did you want to update it with something different?

stefanjudis · 2016-11-23T09:20:05Z

For me the main goal is to make it really beginner accessible, where I still think it could be better. But you did a great job and let's not open up the scope more. I'll open up a follow up then.

I'll check the functionality and do a test run in the next 2 hours and then let's see. :)

Khaledgarbaya · 2016-11-23T09:32:09Z

README.md

+	'entry_key': '_key',
+	'entry_extension': 'md',
+	'contentful': {
+		'content_type': '2wKn6yEnZewu2SCCkus4as'


I would change this to CONTENT_TYPE_ID to make it clear to users that this field can be configurable

Yeah, was wondering about that. Is that fetching all the data under that content_type_id?

@cameronroe yes it is fetching entries under that contentType

Cool. What would be the best way to configure fetching all data from Contentful for users who want to build a site with multiple content_types? Should that be an array then instead of a string?

stefanjudis · 2016-11-28T18:38:46Z

Sorry for the delay here, I'll really try to get it done this week. :(

stefanjudis · 2016-11-30T16:08:55Z

So I gave it a try. Sorry again for the delay. :(

There are a few things I noticed.

Global configuration and documentation

I ran the PR with the following configuration:

  .use(contentful({
    space_id: 'cev1c388opcd',
    access_token: 'efd364e771f86ac45765d805130b12a4dfe638fbb3677fcf7489e4fc41a50cc1',
    'entry_key': '_key',
    'entry_extentsion': 'md',
  }))

and this leads to an exception

> node build.js

{ space_id: 'cev1c388opcd',
  access_token: 'efd364e771f86ac45765d805130b12a4dfe638fbb3677fcf7489e4fc41a50cc1',
  entry_key: '_key',
  entry_extentsion: 'md',
  metadata: {} }
undefined
/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/build.js:50
    if (err) throw err
             ^
TypeError: Cannot read property 'content_type' of undefined
    at Object.getEntriesQuery (/Users/stefanjudis/Projects/cf-contentful-metalsmith/lib/util.js:43:14)
    at Object.createFilesFromEntries (/Users/stefanjudis/Projects/cf-contentful-metalsmith/lib/processor.js:172:22)
    at Ware.<anonymous> (/Users/stefanjudis/Projects/cf-contentful-metalsmith/index.js:44:24)
    at Ware.<anonymous> (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/wrap-fn/index.js:45:19)
    at next (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/ware/lib/index.js:85:20)
    at Ware.run (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/ware/lib/index.js:88:3)
    at Metalsmith.<anonymous> (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/thunkify/index.js:43:12)
    at next (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/co/index.js:90:21)
    at Metalsmith.<anonymous> (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/co/index.js:45:5)
    at Metalsmith.next (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/co/index.js:90:21)
    at Metalsmith.<anonymous> (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/co/index.js:93:18)
    at Immediate.<anonymous> (/Users/stefanjudis/Projects/cf-contentful-metalsmith-pr-tryout/node_modules/co/index.js:52:14)
    at runCallback (timers.js:637:20)
    at tryOnImmediate (timers.js:610:5)
    at processImmediate [as _immediateCallback] (timers.js:582:5)

It was not clear to me, that I have to set a contentful property in the global configuration for the contentful plugin. Having this contentful property in the configuration for the contentful metalsmith plugin feels weird to me, too...

entry_key and entry_extension definitely need to be better documented – I didn't understand what this means first. And had to looks at the source.

I feel like improved documentation with a step by step guide is needed. I don't mind opening up two documents with "How to's"...

Code duplication

When reading the code I noticed a few duplications that can be avoided.

  return client.getEntries(query)
    .then(entries => filterEntries(entries.items, options))
    .then(entries => mapEntriesForFile(entries, file, options))
    .then(entries => processEntriesForFile(file, entries, options))
    .then(entries => getCommonContentForSpace(entries, options))

and

  return client.getEntries(query)
    .then(entries => mapEntriesForFile(entries.items, file, options))
    .then(entries => processEntriesForFile(file, entries, options))
    .then(entries => getCommonContentForSpace(entries, options))

this could be resolved by restructuring the index file a bit.

  // i think this functionality should go into the promise chain
  // and it should really only generating the "files" coming from contentful
  // and then continuing using `processFile` -> this would avoid the duplication
    if (options.entry_key) {
      return processor.createFilesFromEntries(options)
        .then((fileMaps) => {
          console.log(fileMaps);
          Object.assign(files, fileMaps)

          done()
        })
        .catch(handleError.bind(null, done))
    }

    return new Promise(resolve => {
      resolve(Object.keys(files))
    })
    .then(fileNames => {
      return Promise.all(
        fileNames.map(fileName => {
          files[fileName]._fileName = fileName

          return processor.processFile(files[fileName], options)
        })
      )
    })
    .then((fileMaps) => {
      fileMaps.forEach(map => {
        Object.assign(files, map)
      })

      done()
    })
    .catch(handleError.bind(null, done))
  }

Overall I feel like we have a conceptual issue (the contentful property) here and I'd like to go over the possible configuration options again to come to a common denominator.

I really appreciate your work and would like to keep it going. :) Thanks.

ninjasort · 2016-12-07T14:02:47Z

@stefanjudis Sure. The contentful property was also somewhat confusing. Maybe it would be better to keep consistent with the entry_ naming and specify entry_content_types? This could be an array of all content_types that get pulled from the contentful backend and populated as files?

use(contentful({
    space_id: 'cev1c388opcd',
    access_token: 'efd364e771f86ac45765d805130b12a4dfe638fbb3677fcf7489e4fc41a50cc1',
    entry_key: '_key',
    entry_extentsion: 'md',
    entry_content_types: [
      '2wKn6yEnZewu2SCCkus4as',
      '4fjsk3jfl5jghsl402jflsh3lhgs'
    ]
  }))

ninjasort · 2017-01-01T06:51:41Z

@stefanjudis Any thought on that idea?

stefanjudis · 2017-01-02T09:48:54Z

@cameronroe Yeah, I think this makes way more sense! 👍

ismay · 2017-10-01T19:20:41Z

Hey, just wondering if this pr is still considered for merging?

ninjasort · 2017-10-05T18:13:02Z

This hasn't been looked at in months. Any ideas @stefanjudis ?

stefanjudis · 2017-10-06T07:27:24Z

@cameronroe Oh well... sorry about that. 😢

That slipped through. I'll have a look again asap.

ninjasort · 2017-10-08T21:16:16Z

No worries! Let me know how I can help. I may also need to review what was written here.. 🙃

Cameron J Roe added 8 commits November 13, 2016 01:55

feat(processor): Add entry_key and entry_extension properties

e7341c0

docs(global config): Add entry_key, entry_extension to global config,…

e502960

… ref contentful#36

test(createFilesFromEntries): catch promise errors

c477a9f

test(markdown): test markdown contents

a9f0184

docs(readme, global-settings): update contentful config for docs

1516563

docs(readme, global-settings): fix quotes

d377519

docs(global settings): should link to source settings

46d6372

docs(global settings): should link to source settings

1f8bc9c

ninjasort changed the title ~~Adds support for global key, #36~~ Adds support for global key Nov 18, 2016

stefanjudis reviewed Nov 18, 2016

View reviewed changes

Cameron J Roe added 4 commits November 18, 2016 14:17

test(only): remove solo test

42e5bb8

docs(jsdoc): update jsdocs

9f28d95

test(linting, jsdocs): fix linting errors, add jsdoc

a8602b5

test(test dir): add empty test fixture

939ad45

test(gitkeep): fix issues with gitkeep

240f079

stefanjudis reviewed Nov 21, 2016

View reviewed changes

docs(readme): update readme configs

497b4b2

Khaledgarbaya reviewed Nov 23, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for global key #42

Adds support for global key #42

ninjasort commented Nov 18, 2016 •

edited

Loading

stefanjudis commented Nov 18, 2016

stefanjudis left a comment

stefanjudis Nov 18, 2016

stefanjudis commented Nov 21, 2016

ninjasort commented Nov 21, 2016

stefanjudis left a comment

stefanjudis Nov 21, 2016

ninjasort Nov 22, 2016

stefanjudis Nov 21, 2016

stefanjudis Nov 21, 2016

ninjasort commented Nov 22, 2016

stefanjudis commented Nov 22, 2016 •

edited

Loading

ninjasort commented Nov 23, 2016

stefanjudis commented Nov 23, 2016

Khaledgarbaya Nov 23, 2016

ninjasort Nov 23, 2016

Khaledgarbaya Nov 24, 2016

ninjasort Nov 24, 2016

stefanjudis commented Nov 28, 2016

stefanjudis commented Nov 30, 2016

ninjasort commented Dec 7, 2016 •

edited

Loading

ninjasort commented Jan 1, 2017

stefanjudis commented Jan 2, 2017

ismay commented Oct 1, 2017

ninjasort commented Oct 5, 2017

stefanjudis commented Oct 6, 2017

ninjasort commented Oct 8, 2017

Adds support for global key #42

Are you sure you want to change the base?

Adds support for global key #42

Conversation

ninjasort commented Nov 18, 2016 • edited Loading

stefanjudis commented Nov 18, 2016

stefanjudis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanjudis commented Nov 21, 2016

ninjasort commented Nov 21, 2016

stefanjudis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninjasort commented Nov 22, 2016

stefanjudis commented Nov 22, 2016 • edited Loading

ninjasort commented Nov 23, 2016

stefanjudis commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanjudis commented Nov 28, 2016

stefanjudis commented Nov 30, 2016

Global configuration and documentation

Code duplication

ninjasort commented Dec 7, 2016 • edited Loading

ninjasort commented Jan 1, 2017

stefanjudis commented Jan 2, 2017

ismay commented Oct 1, 2017

ninjasort commented Oct 5, 2017

stefanjudis commented Oct 6, 2017

ninjasort commented Oct 8, 2017

ninjasort commented Nov 18, 2016 •

edited

Loading

stefanjudis commented Nov 22, 2016 •

edited

Loading

ninjasort commented Dec 7, 2016 •

edited

Loading