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

[BUGFIX release] createRecord should only setup relationships it has … #5048

Merged
merged 3 commits into from
Jul 11, 2017

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Jul 2, 2017

…data for

relationships should in theory now only be accessed if they are used (set or get). This should mitigate the large number of test failures see during upgrade due to to

internalModel.type.typeForRelationship(relationshipMeta.key, store);

If this doesn't actually fix the issue in enough cases, we can always turn that assert into a warn or something. But I would love a failing test for the exact case that remains (if one exists) to be 100% sure. As I would prefer to not loose a good assertion unless we must.

@stefanpenner stefanpenner force-pushed the only-setup-relationships-with-data branch 2 times, most recently from c6485ee to d52c3e8 Compare July 2, 2017 23:30
Copy link
Member

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

LGTM; a minor assertion cleanup that would be nice but the change itself is 👍

let book = store.createRecord('book', { name: 'The Greatest Book' });
let relationship = book._internalModel._relationships.get('author');

assert.equal(relationship.hasData, false, 'relationship has data');
Copy link
Member

Choose a reason for hiding this comment

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

assertion message is wrong: relationship does not have data

});

let relationship = book._internalModel._relationships.get('author');
assert.equal(relationship.hasData, false, 'relationship has data');
Copy link
Member

Choose a reason for hiding this comment

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

see ^

let page = store.createRecord('page');

let relationship = chapter._internalModel._relationships.get('pages');
assert.equal(relationship.hasData, false, 'relationship has data');
Copy link
Member

Choose a reason for hiding this comment

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

see ^

let chapter = store.createRecord('chapter', { title: 'The Story Begins' });
let relationship = chapter._internalModel._relationships.get('pages');

assert.equal(relationship.hasData, false, 'relationship has data');
Copy link
Member

Choose a reason for hiding this comment

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

see ^

@hjdivad hjdivad force-pushed the only-setup-relationships-with-data branch from d7280b8 to db42a28 Compare July 3, 2017 01:41
@stefanpenner stefanpenner force-pushed the only-setup-relationships-with-data branch from db42a28 to f741f34 Compare July 3, 2017 05:24
@stefanpenner stefanpenner force-pushed the only-setup-relationships-with-data branch from f741f34 to 8628f2a Compare July 3, 2017 18:46
@stefanpenner
Copy link
Member Author

stefanpenner commented Jul 3, 2017

did a rebase now, seems to be blocked by:

SyntaxError: ember-data/adapters/json-api.js: @ember/debug does not have a deprecate import
   7 | import RESTAdapter from "./rest";
   8 | import { isEnabled } from '../-private';
>  9 | import { deprecate } from '@ember/debug';
     | ^
  10 | import { instrument } from 'ember-data/-debug';

@rwjblue is investigating.

@stefanpenner stefanpenner force-pushed the only-setup-relationships-with-data branch 2 times, most recently from 605fcae to 8628f2a Compare July 3, 2017 20:51
@stefanpenner
Copy link
Member Author

@runspired r?

@runspired
Copy link
Contributor

LGTM

@stefanpenner
Copy link
Member Author

@bmac got cycles for a backport + release? (I'm currently on vacation, but if you don't have cycles I can handle it)

@bmac bmac merged commit 3eb760b into master Jul 11, 2017
@bmac bmac deleted the only-setup-relationships-with-data branch July 11, 2017 13:30
@bmac
Copy link
Member

bmac commented Jul 11, 2017

@stefanpenner I'm on it.

bmac pushed a commit that referenced this pull request Jul 11, 2017
#5048)

* [BUGFIX release] createRecord should only setup relationships it has data for

* update deps

(cherry picked from commit 3eb760b)
bmac pushed a commit that referenced this pull request Jul 11, 2017
#5048)

* [BUGFIX release] createRecord should only setup relationships it has data for

* update deps

(cherry picked from commit 3eb760b)
@bmac
Copy link
Member

bmac commented Jul 11, 2017

Released as 2.14.4.

@stefanpenner
Copy link
Member Author

@bmac awesome thanks dude, you rock!

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.

None yet

4 participants