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

added morphValues for morphTo relation #1326

Merged
merged 8 commits into from Jul 21, 2017

Conversation

Projects
None yet
7 participants
@chamini2
Copy link
Contributor

chamini2 commented Jul 21, 2016

This addresses #1324

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Jul 21, 2016

Hey, @vellotis, I started working on this but I'm not being able to get all the tests to run. So I would appreciate it if you could take a look?

@vellotis

This comment has been minimized.

Copy link
Contributor

vellotis commented Aug 11, 2016

Huh. Sry @chamini2 . I have somehow missed this PR. I did rerun the tests. There seem to be issues with the PR.

});
if (!Target) {
morphCandidate: function(candidates, morphValue) {
const tuple = _.find(candidates, (tuple) => morphValue === tuple[1]);

This comment has been minimized.

@vellotis

vellotis Aug 18, 2016

Contributor

I would use ES6 array destructing...

const [Target] = _.find(candidates, ([,tableName]) => tableName === morphValue)
@@ -74,8 +74,24 @@ module.exports = function (bookshelf) {
// can't include it with the rest of the relational methods.
const morphTo = Model.prototype.morphTo;
Model.prototype.morphTo = function(relationName) {

This comment has been minimized.

@vellotis

vellotis Aug 18, 2016

Contributor

I don't really like that the API of morphTo gets slightly changed. But I don't have any better idea to solve it.

This comment has been minimized.

@vellotis

vellotis Aug 18, 2016

Contributor

Ok. I'm good with it.

@vellotis

This comment has been minimized.

Copy link
Contributor

vellotis commented Aug 18, 2016

Seems that there is an issue with eager loads morphTo (photos -> imageable) test.
Somewhy imageable doesn't get wetched.

if (_.isArray(arguments[1])) {
columnNames = arguments[1];
if (_.isArray(arguments[1]) || _.isNil(arguments[1])) {
columnNames = arguments[1] || null;

This comment has been minimized.

@vellotis

vellotis Aug 18, 2016

Contributor

As it is not a registry plugin I would check for strings instead of using isNil. This way the API would be intact.

if (_.isArray(arguments[1]) && arguments[1].every(_.isString)) {
  columnNames = arguments[1];

This comment has been minimized.

@chamini2

chamini2 Aug 21, 2016

Author Contributor

This would mean both:

let Photo = bookshelf.Model.extend({
  tableName: 'photos',
  imageable: function() {
    return this.morphTo('imageable', ["ImageableType", "ImageableId"], [Post, "cover"], Site);
  }
});

and

let Photo = bookshelf.Model.extend({
  tableName: 'photos',
  imageable: function() {
    return this.morphTo('imageable', [Post, "cover"], Site);
  }
});

Would be valid?

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Aug 21, 2016

Hey @vellotis, now the tests pass! It was failing in the eager loading because was comparing table names against morphValue

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Aug 21, 2016

Now I pass the morphValue here (src/eager#63) to be used here (src/relations#121)

@chamini2 chamini2 changed the title added morphValues for morphTo relation [WIP] added morphValues for morphTo relation Aug 21, 2016

} else {
columnNames = null;
candidates = _.drop(arguments);
}

This comment has been minimized.

@vellotis

vellotis Aug 23, 2016

Contributor

I still think that we cannot change the core Model API.

// This should be possible when "registry" plugin is not used
this.morphTo('imageable', null, Site, Author)

It is used by many Bookshelf users. Actually I think it is used also with "registry" plugin. So the best would be that it is not changed for that too. But as there is no way to determine for 2nd argument if the array is the declaration for column names or for Model name and morphValue.

So I would suggest to refactor following logic.

if (_.isArray(arguments[1])) {
  columnNames = arguments[1];
} else {
  columnNames = null;
}
if (_.isArray(arguments[1]) || _.isNil(arguments[1])) {
  candidates = _.drop(arguments, 2);
 } else {
  candidates = _.drop(arguments, 1);
}

and implement it to look the first array argument to be string as I did indicate previously.

if (_.isArray(arguments[1]) && arguments[1].every(_.isString)) {
  columnNames = arguments[1];
  candidates = _.drop(arguments, 2);
} else {
  columnNames = null;
  candidates = _.drop(arguments, 1);
}

This comment has been minimized.

@chamini2

chamini2 Aug 23, 2016

Author Contributor

Hey, but these changes do not change the original API, only change it if the morph values are specified (which wasn't part of the API before).

The call this.morphTo('imageable', Site, Author) continues to be valid (the registry plugin version also continues to be valid), the condition isArray(arguments[1]) will give false, which effectively drops only one argument (imageable) and sets column names as null

This comment has been minimized.

@vellotis

vellotis Aug 23, 2016

Contributor

In original API the second parameter as columnNames is optional. Previously the given parameter was the only type of array. Now as you are using arrays to declare morphValue to you can not make columnNames optional.

This comment has been minimized.

@chamini2

chamini2 Aug 23, 2016

Author Contributor

I think I'm not conveying my point. I'll give some examples with their results to see if we agree:

morphTo('imageable', 'Site', 'Author')
// columnNames: null
// morphValues: ['Site', 'sites'] ['Author', 'authors' ]

morphTo('imageable', 'Site', ['Author', 'profile_pic'])
// columnNames: null
// morphValues: ['Site', 'sites'] ['Author', 'profile_pic' ]

morphTo('imageable', null, ['Site', 'cover_photo'], ['Author', 'profile_pic'])
// columnNames: null
// morphValues: ['Site', 'cover_photo'] ['Author', 'profile_pic' ]

morphTo('imageable', ['Site', 'cover_photo'], ['Author', 'profile_pic'])
// columnNames: ['Site', 'cover_photo']
// morphValues: ['Author', 'profile_pic' ]

morphTo('imageable', null, 'Site', ['Author', 'profile_pic'])
// columnNames: null
// morphValues: ['Site', 'sites'] ['Author', 'profile_pic' ]

NOTE: these examples all use the registry plugin, but would have the same behaviour passing the model objects.

So no old code would break (first example), but we need null for specifying morphValues for each model.

What would you have expected differently from these or am I missing some case?

This comment has been minimized.

@vellotis

vellotis Aug 23, 2016

Contributor

consider this one without registry plugin:

morphTo('imageable', [Site, 'cover_photo'], [Author, 'profile_pic'])
// columnNames: [Site, 'cover_photo']
// morphValues: [Author, 'profile_pic']

Actually, yes, it doesn't reflect to old API. I am sorry. But it would make new API more fail proof.

This comment has been minimized.

@chamini2

chamini2 Aug 23, 2016

Author Contributor

I don't know if making the registry and non-registry calls work differently is a good idea.

Making these:

morphTo('imageable', [Site, 'cover_photo'], [Author, 'profile_pic'])
// columnNames: null
// morphValues: [Site, 'cover_photo'] [Author, 'profile_pic']

morphTo('imageable', ['Site', 'cover_photo'], ['Author', 'profile_pic'])
// columnNames: ['Site', 'cover_photo']
// morphValues: ['Author', 'profile_pic']

would make migrating from a non-registry code to code with registry more difficult.

candidates = drop(arguments, 2);
} else {
candidates = drop(arguments, 1);
}

This comment has been minimized.

@vellotis

vellotis Aug 23, 2016

Contributor

This could be optimized to

if (isArray(arguments[1]) || isNil(arguments[1])) {
  columnNames = arguments[1]; // may be `null`
  candidates = drop(arguments, 2);
} else {
  columnNames = null;
  candidates = drop(arguments, 1);
}

This comment has been minimized.

@chamini2

chamini2 Aug 23, 2016

Author Contributor

I actually had it like that before and thought it would be more readable this way, I can change it back, what do you think?

This comment has been minimized.

@vellotis

vellotis Aug 23, 2016

Contributor

No offence. I had more trouble to read it this way.

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Aug 30, 2016

Ok, the following message was sent in the Gitter chat but had no response, so I'm pasting it here to see if we get some responses:


Hello everyone,

I've been working in the PR #1326 to add a morphValues option for the morphTo relation. The idea is to implement the counterpart of the morphValue option for morphOne and morphMany.

The PR is ready functionality wise but @vellotis and I have been discussing the new API for the morphTo relation and we would like some other opinions in the subject, and would like if the addition didn't break any old Bookshelf code:

Passing the target model and morphValue in an array of size two, kind of like columnNames (i.e. [Author, 'profile_pic']). For this options, keep the registry plugin in mind.

This API could have two different behaviours with columnNames (which is also an array):

Option 1

To set morphValues and no columnNames: pass null in columnNames.

morphTo('imageable', null, ['Site', 'cover_photo'], ['Author', 'profile_pic'])
// columnNames: null
// morphValues: ['Site', 'cover_photo'] ['Author', 'profile_pic' ]

This option always knows which are morphValues and which are columnNames without a doubt.

Option 2

To set morphValues and no columnNames: simply pass the morphValues.

morphTo('imageable', ['Site', 'cover_photo'], ['Author', 'profile_pic'])
// columnNames: null
// morphValues: ['Site', 'cover_photo'] ['Author', 'profile_pic' ]

This option has to test 'Site' as a model in the registry plugin and when it successfully finds it, decides it's not a columnNames.

If we pass a columnNames:

morphTo('imageable', ['imgtype', 'imgid'], ['Site', 'cover_photo'], ['Author', 'profile_pic'])
// columnNames: ['imgtype', 'imgid']
// morphValues: ['Site', 'cover_photo'] ['Author', 'profile_pic' ]

This tests 'imgtype' as a model in the registry plugin and when it doesn't finds it, decides it's a columnNames.

When using the registry plugin, you could specify there are no columnNames using the null to be explicit about it.


Pros & Cons

We should also consider which one is easier to write documentation for.

Option 1

  • + No trick to it, just works as expected
  • - More verbose

Option 2

  • + Less verbose
  • - Could ignore some programmer errors (passing a model with a typo and the registry plugin assumes is the columnNames)

Please comment any pro or con I didn’t consider, or any better options!

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Aug 30, 2016

How about a ❤️ for option 1 and 🎉 for option 2 as a voting system? Or comment if you prefer.

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Oct 3, 2016

Went for option 🎉.

@daleknauss

This comment has been minimized.

Copy link

daleknauss commented Feb 6, 2017

Has there been any movement on this feature and/or alternatives found? I find myself with an identical need and not sure how to proceed.

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Mar 13, 2017

Bump!

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Jul 19, 2017

Bump for new maintainers: @Playrom. I would be glad to see this in Bookshelf.

@Playrom Playrom self-requested a review Jul 19, 2017

@Playrom

This comment has been minimized.

Copy link
Contributor

Playrom commented Jul 19, 2017

hi @chamini2 !

I will look at this as soon as possible, I see that you have already provided us with a description similar to our new PR-Templates, this will help us a lot in the analysis!

in the mean time can you commit some edits to make the tests pass? I think you just have to rebase your branch to our current master

@Playrom Playrom self-assigned this Jul 19, 2017

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Jul 19, 2017

Done, @Playrom. I had to update some expected results for the newly added DBMS.

@dj-hedgehog

This comment has been minimized.

Copy link
Member

dj-hedgehog commented Jul 20, 2017

Good job @chamini2. Can you also add documentation for the feature?

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Jul 20, 2017

Is that alright, @dj-hedgehog @Playrom?

@Playrom

This comment has been minimized.

Copy link
Contributor

Playrom commented Jul 20, 2017

yes it's ok
Only one thing, I'm a bit concerned for the change to the morphTo api, I don't want to break existing usage....

have you tried to test it without your update to the integrations test?

@Playrom Playrom removed their request for review Jul 20, 2017

@chamini2

This comment has been minimized.

Copy link
Contributor Author

chamini2 commented Jul 20, 2017

Well, the idea is to change the morphTo API to accept this new option, but all previous use cases would work the same. As you can see in the tests, I only added the option to some relations, to show that it's used only where needed.

@Playrom Playrom added relations and removed performance labels Jul 21, 2017

@Playrom Playrom merged commit 1ac4006 into bookshelf:master Jul 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chamini2 chamini2 deleted the chamini2:morphTo-morphValues branch Jul 21, 2017

@racheldelman

This comment has been minimized.

Copy link

racheldelman commented Sep 11, 2017

@Playrom any idea when this will be released?

@Playrom

This comment has been minimized.

Copy link
Contributor

Playrom commented Sep 20, 2017

in the next few weeks I think

@mohammedBalhaddad

This comment has been minimized.

Copy link

mohammedBalhaddad commented Oct 19, 2017

@Playrom any update for the release ? thanks a lot

lyfeyaj added a commit to lyfeyaj/bookshelf that referenced this pull request Nov 6, 2017

Merge https://github.com/bookshelf/bookshelf
* https://github.com/bookshelf/bookshelf: (43 commits)
  Fixed random test failure in test/integration/model.js
  Update .npmignore to exclude non-prod files
  Fix bookshelf#1662: move appropriate files into .github directory
  chore(package): update babel-eslint to version 8.0.1
  Add js formatting to "How do I debug?" at README
  Revert "Update dependencies to enable Greenkeeper 🌴 (bookshelf#1616)"
  Update dependencies to enable Greenkeeper 🌴 (bookshelf#1616)
  small registry plugin refactoring for easier read
  Build Project before testing in "npm test" script
  Update README.md (bookshelf#1537)
  New implementation for the setting of timestamps columns values
  Update CONTRIBUTING.md due to changes in MySQL 5.7 (bookshelf#1610)
  use OracleDB tests only when oracledb is installed (bookshelf#1609)
  added morphValues for morphTo relation (bookshelf#1326)
  Rename ISSUE-TEMPLATE.md to ISSUE_TEMPLATE.md
  Rename PR-TEMPLATE.md to PULL_REQUEST_TEMPLATE.md
  Revision to the contributing guidelines (bookshelf#1601)
  npm install knex@0.13 in the README (bookshelf#1604)
  Link to the bookshelf#1600 discussion in README
  Allow passing of time via options (bookshelf#1592)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.