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

Possibility to provide default values for the entity #117

Closed
wants to merge 1 commit into from
Closed

Possibility to provide default values for the entity #117

wants to merge 1 commit into from

Conversation

smashercosmo
Copy link
Contributor

This PR fixes #114.

  • Tests updated
  • Readme updated
  • Getting schemaAssignEntity is moved out of the loop for performance reasons

@paularmstrong
Copy link
Owner

paularmstrong commented May 26, 2016

Looks good! Just one small issue — the following test will fail:

  it('does not overwrite the default', function () {
    var article = new Schema('articles', {defaults: {isFavorite: false}}),
      input;

    input = {
      id: 1
    };

    Object.freeze(input);

    normalize({ id: 2, title: 'foo' }, article);

    normalize(input, article).should.eql({
      result: 1,
      entities: {
        articles: {
          1: {
            id: 1,
            isFavorite: false
          }
        }
      }
    });
  });

let normalized = {};
const defaults = schema && schema.getDefaults && schema.getDefaults();
const schemaAssignEntity = schema && schema.getAssignEntity && schema.getAssignEntity();
let normalized = isObject(defaults) ? defaults : {};
Copy link
Owner

Choose a reason for hiding this comment

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

To fix the test I noted, I believe you just need to make a new object that includes the default values:

let normalized = isObject(defaults) ? { ...defaults } : {};
// or
let normalized = isObject(defaults) ? Object.assign({}, defaults) : {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. My bad (( didn't think about object reference. I'll go with spread, because as I understand Object.assign should be polyfilled.

@smashercosmo
Copy link
Contributor Author

PR updated

@smashercosmo
Copy link
Contributor Author

@paularmstrong ping

@paularmstrong
Copy link
Owner

@smashercosmo (sorry, been on holiday) Looks good. I'll mark it to be merged and in the next release. I'd like to hold off on merging for now so that the documentation doesn't add any confusion as to why it's not available yet.

@smashercosmo
Copy link
Contributor Author

Any news on this?

@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.

Possibility to provide default values to a model
2 participants