kontent-ai/sample-app-express-js

The Dancing Goat demo site created using Express.js and Pug templates using Kontent.ai as a data source.

sample-app-express-js's Issues

Repository as classes

Is there any reasons you are using functions for repositories? Why not use standard classes? Classes are supported for a long time now in javascript (https://javascript.info/class)

With that you would also be able to remove these strange lines of code:

if (!(this instanceof CoffeeRepository)) return new CoffeeRepository();

Repo name (minor)

Excellent work getting this updated for the rebranding! One minor thing regarding the repo name though. To fall in line with the naming conventions we're using this should be named kontent-sample-app-expressjs

Two sdk version installed

There are 2 js sdk versions installed - https://github.com/Kentico/cloud-expressjs-app/blob/master/package.json#L20

Remove the old one.

Use eslint for code consistency and quality

To help you write a code, I would highly recommend using eslint (unless you want to switch to typescript :-)) - https://eslint.org/

We are using eslint in our sample react app (https://github.com/Kentico/cloud-sample-app-react), but you should probably need slightly different set of rules as those are made for react. Default rules would be a good start nevertheless.

Upsert Errors when Content Type has a URL Slug

If the content type has a URL Slug the upsert fails with the following error:

"The URL slug element is missing the 'mode' property. Provide either 'autogenerated' or 'custom' for the 'mode' value."

Solve eslint issues

Eslint is used to 'enforce' good coding practices so if you have to disable it for certain lines, you have to have a good reason. Currently, there is a lot of comments like this https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L77 that disable eslint. Rather than using this, try to actually fix the code.

Implement typeResolvers for sdk

You are using implicitly typed models and it would be nice to use strongly typed models in case you would want to use rich text resolving, link resolving or maybe extends classes with additional methods.

Unify const + let and remove var

In some cases you are using const such as:

const { Observable, defer } = require('rxjs');

But in body of your code you are almost exclusively using var. Is there any reason for this? Since you can use const, use it where appropriate and same for let. Do not use var at all.

Naming convention

Naming is somewhat inconsistent, for example:

var DeliveryClient = require('../delivery');
var LinkResolver = require('../resolvers/LinkResolver');

../delivery is exported as an instance of DeliveryClient so it should be camelCase. Filename is correctly camelCase.

../resolvers/LinkResolver is exported as function so it should be camelCase as well. Filename is for some reason Pascal case, but it should be camelCase.

I guess it doesn't matter what naming convention you use as much as having some consistency. Generic rules are:

camelCase for variables and functions, PascalCase for types(classes), and UPPERCASE_SNAKE_CASE for constants.

For filenames I would use lowercase only + dashes to split up some words (e.g. 'aboutus' -> 'about-us' https://github.com/Kentico/cloud-expressjs-app/blob/bf3344636da309d4e8491d2da3fe3f595383a4fd/routes/aboutus.js)

Create eslint script and run it before build + use only local dependencies

Eslint should be run as a command when you build/before publish etc.. for that you can create script in your package.json.

I'm using tslint (I assume eslint is very similar) this way:

"ts-lint-local": "./node_modules/.bin/tslint",
"ts-lint:run": "npm run ts-lint-local

Also, user should ideally not be expected to install any global dependencies in order to run your app. All dependencies that you need to use to build / run the app should refer to local dependencies in your node modules. You can see an example of my tslint above.

Express a devDependency?

I believe express should be a devDependency as it is the server, right?

Extract API keys definition to .env

Motivation

It is easily possible to commit API keys as a part of the pull request in delivery.js file.

Currently, it is possible to configure the project manually

When you configure your project manually it is required to temporarily change the source code in delivery.js file and that could lead to committing this unintended change that was used for, let's say testing purposes.

Proposed solution

If the project ID was loaded from the environment, as an environment variable, it would eliminate the possibility for unintended commit to delivery.js.
Loading environment variables is possible utins dotenv library.

Best practice nowadays is to use environment variables stored in .env file.

• Set up .gitignore to ignore .env file
• Configure delivery.js to use .env definition for key (projectId )
• Update Readme

Additional context

• Highly used is dotenv library: https://www.npmjs.com/package/dotenv
• Related to
  • kontent-ai/sample-app-vue#23
  • kontent-ai/sample-app-react#134

Incorrect implementation of observable pattern

Implementation of observable pattern is sadly incorrect.

Following is based on https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L14 .

As far as I understand it, the ensureItems method should return observable (maybe name it more appropriate) because you are subscribing it in https://github.com/Kentico/cloud-expressjs-app/blob/master/routes/coffee.js#L9, however you are not working with the response so the subscription doestn't do anything because data are actually resolver in https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L31

So basically you cannot use subscribe in ensureItems, but rather you should always use valid observable that will load data from KC (= no need for createDummyObservable).

You can use RxJS's map operator if you need to transform data inside ensureItems and still return observable.

Do not call subscribe inside a subscribe (https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L35). Instead you should use dedicated RxJS operators such as zip, flatMap, switchMap or something similar based on your needs. If the data does not depend on each other and you want them resolved simultaneously, you would probably want to use zip (https://www.learnrxjs.io/operators/combination/zip.html)

Why are you calling this? https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L37 This code should not be there and you should unsubscribe when you switch pages. There should be some event that gets triggered when you go from page to page so that you can notify your repository to cancel requests. Best implementation is done with takeUntil as can be discussed here: https://medium.com/@benlesh/rxjs-dont-unsubscribe-6753ed4fda87

Why are you calling defer (https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L19) ? Defer should not be necessary, you only need to return one single observable that will contain data (e..g items) that you wont to work with and then in https://github.com/Kentico/cloud-expressjs-app/blob/master/routes/coffee.js#L10 you extends 'subscribe' with parameter that will contains your data.

Calling complete is not necessary (https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L38)

The method in https://github.com/Kentico/cloud-expressjs-app/blob/master/routes/coffee.js#L7 doesn't do anything as the actual logic is performed inside the ensure items and this observable doesn't really need to be there (but it should! As the logic should be moved here from ensureItems). Thats why your app still works even when you return an empty observable (https://github.com/Kentico/cloud-expressjs-app/blob/master/repositories/coffee-repository.js#L14)

Sorry if these comments are misplaced or difficult to read. I didn't really know how to describe it better. If you have any questions, feel free to let me know. I would recommend trying to 'fix' this particular use case before moving out and fixing the rest.

Prepare automatic deployment

Motivation

Currently, there is no check if changes des not break the site.

Proposed solution

Implement automatic deployment with the preview deployment for PRs - ideally to Netlify.

Additional context

Add any other context, screenshots, or reference links about the feature request here.

Translated text/rich text elements in Content Type Snippets

Motivation

Why is this feature required? What problems does it solve?

Currently, text/rich text elements which are part of a Content-Type Snippet, are not translated.

Proposed solution

To include the data of content snippets, it would need to loop through the content type's elements and call the snippet endpoint for each one. Then, you could add the elements in the response to some array that holds all of the type elements.
It would be similar to here: https://github.com/Kentico/kontent-google-sheets-add-on/blob/master/types.gs#L55.

Unsubscribe from all observables

Whenever you subscribe to an Observable, you also need to unsubscribe when it's no longer required (e.g. user switched to different page, component is destroyed etc...), otherwise you have memory leaks in your app and it can even affect the functionaltity of site (because observables are asynchronous so if you execute 2 requests and the second finishes before first one, it will override the result)

I would implement it with takeUntil (see https://medium.com/@benlesh/rxjs-dont-unsubscribe-6753ed4fda87). You will have to decide when observables should be unsubscribed though as I'm not that familiar the flow of express apps.