Skip to content
This repository has been archived by the owner on Mar 20, 2022. It is now read-only.

Can normalize entities keyed with their ID #155

Merged
merged 1 commit into from
Nov 9, 2016

Conversation

timhwang21
Copy link
Contributor

Problem

It is hard to deal with collections keyed by ID without an ID in the entity itself

Solution

Refactored #73 by to pass test. Allows idAttribute to be passed a key with which to normalize IDs on. Also fixed some instances where collectionKey wasn't being passed to visit(), and added documentation.

@@ -269,7 +275,7 @@ const article = new Schema('articles', { defaults: { likes: 0 } });

### `Schema.prototype.define(nestedSchema)`

Lets you specify relationships between different entities.
Copy link
Owner

Choose a reason for hiding this comment

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

Please preserve trailing whitespace in markdown. Two spaces at the end of a line indicates a line break.

}
}

function normalizeResult(result) {
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 this function? When is the result coming back as an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result comes back as an object because the new functionality is added to an iterable schema (arrayOf(foo) or keyedObjectOf(foo), as mentioned above). Thus, visit() returns the result of visitIterable(), which is an object mapping keys to the result of curriedItemMapper(obj[key], key).

When we use the function (obj, key) => key, this ultimately returns an object something like { 1: '1', 2: '2', 3: '3' } (whereas something like obj => obj would return { 1: foo, 2: bar, 3: baz } or something). Here, the values of the object don't represent objects, so we convert into an array of keys after double checking that all the keys do equal all their values.

Copy link

@specialunderwear specialunderwear Oct 5, 2016

Choose a reason for hiding this comment

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

It does happen that the result can not be normalized, if you specify idAttribute for example, your key will not be your id, and the result can not be further normalized. So every time you don;t want the key to be the ID, but you still need it to compute the id, you will not get an array, but the object instead.


Object.freeze(input);

normalize(input, arrayOf(user)).should.eql({
Copy link
Owner

Choose a reason for hiding this comment

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

arrayOf seems like a poor use for an object. Would it simplify things to add a keyedObject function that assumes your ID is the key for each entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, right now the functionality that the previous author wrote is baked into visitIterable() (line 48). So like valuesOf(), this function would create a new IterableSchema anyways. I do agree that valuesOf() makes more sense, and perhaps keyedValuesOf() more. What do you think about a keyedValuesOf() that's yet another alias of arrayOf()? Not sure if that's unnecessary API complication, though.

// The function is passed both the current value and its key
// This is helpful in occasions where your data is keyed by id, but doesn't contain id inside the value
const keyedByIdJson = { 1: { ... }, 2: { ... }, 3: { ... } };
function generateSlug(entity, value) { return value; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realized this is misleading. Changed to generateSlug(entity, id) { return id; }

@paularmstrong
Copy link
Owner

@timhwang21 Can you fix the merge conflicts when you have a chance? I'd like to get this merged and deployed this week.

@timhwang21
Copy link
Contributor Author

@paularmstrong done!

@paularmstrong paularmstrong merged commit 7d527b6 into paularmstrong:master Nov 9, 2016
@lock
Copy link

lock bot commented May 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the Outdated label May 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants