Skip to content

Conversation

bobbysmith007
Copy link
Contributor

Description

  • by listing loopback-connector as a dependency it will be loaded
    as a submodule by loopback-connector-postgres when used in a
    parent loopback project. Because we reference objects/constructors
    out of loopback-connector, and ask isinstance style questions of
    them, they need to resolve to the same constructor

EG:

  • App uses loopback-connector and loopback-connector-postgres
  • App wishes to generate a loopback-connector.ParameterizedSQL and
    pass it to buildDefinition (a where clause), but it is not the same
    ParameterizedSQL object (even if loaded from the same lib/version),
    because it is in the sub node_packages

NB: this patch should probably be mirrored in loopback-datasource-juggler
To effectively share object models, everyone needs to load the same
library from the same spot.

loopback loads loopback-datasource-juggler (and should also load
loopback-connector) and any particular app will choose the backend
loopback-connector-postgres, we want all of these libs to refer to the
same objects

Testing

Since this only happens in concert with other libraries and dependency chains I was not sure how to add a unit/integration test to this project about it. That said "peerDependencies" seems to have been exactly created for this purpose

Related issues

  • None

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide
@slnode
Copy link

slnode commented Apr 13, 2017

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Apr 13, 2017

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link

slnode commented Apr 13, 2017

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Apr 13, 2017

Can one of the admins verify this patch?

@ssh24
Copy link
Contributor

ssh24 commented Apr 13, 2017

@bobbysmith007 I can't find your branch in https://github.com/AccelerationNet/loopback-datasource-juggler/branches. Any reason?

Could you please rebase your branch and fix the commit linter? Here is the linting error:

************************************************** ** ** Linting commit logs ** ** 1 problems found: ** 0661c6c - Add loopback-connector into peer dependencies: line 16 longer than 72 characters. ** ************************************************** 

Linting rule:

  • The first line of the commit message should not be greater than 50 characters.
  • Every subsequent line should not be greater than 72 characters.

NB: this patch should probably be mirrored in loopback-datasource-juggler
To effectively share object models, everyone needs to load the same
library from the same spot.

Could you also submit a PR to fix this in loopback-datasource-juggler?

@bobbysmith007
Copy link
Contributor Author

Here is the other PR:

loopbackio/loopback-datasource-juggler#1326

https://github.com/AccelerationNet/loopback-datasource-juggler/tree/fix-connector-dependency

I can amend the patch on the morning but I'm away from my workstation for the day

@ssh24 ssh24 force-pushed the fix-parameterized-sql branch from 0661c6c to 6e70466 Compare April 13, 2017 22:16
@ssh24
Copy link
Contributor

ssh24 commented Apr 13, 2017

@bobbysmith007 Ah sorry I was trying to look for branch fix-parameterized-sql on your forked loopback-datasource-juggler. No worries, I rebased the code for you. Please pull before committing any further changes.

@ssh24
Copy link
Contributor

ssh24 commented Apr 13, 2017

@slnode test please

Copy link
Contributor

@ssh24 ssh24 left a comment

Choose a reason for hiding this comment

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

LGTM

@ssh24 ssh24 self-assigned this Apr 13, 2017
@ssh24 ssh24 added the apex label Apr 13, 2017
@ssh24 ssh24 added this to the Sprint 33 - Apex milestone Apr 13, 2017
@ssh24 ssh24 merged commit cc0b806 into loopbackio:master Apr 13, 2017
ssh24 pushed a commit that referenced this pull request Apr 19, 2017
ssh24 pushed a commit that referenced this pull request Apr 19, 2017
Add loopback-connector as peer dependencies (#246) This reverts commit cc0b806.
ssh24 pushed a commit that referenced this pull request Apr 19, 2017
Add loopback-connector as peer dependencies (#246) This reverts commit cc0b806.
@zbarbuto
Copy link
Contributor

Why was this reverted?

@ssh24
Copy link
Contributor

ssh24 commented Apr 20, 2017

There were some issues with the installation of loopback-connector-postgresql on a lb app which does not automatically install loopback-connector as a dependency. We are currently discussing the pros and cons of using peerDependencies and will start using them if properly applicable in our modules.

kjdelisle pushed a commit that referenced this pull request May 15, 2017
 * Add docker setup (#256) (Sakib Hasan) * Allow explicit numeric datatype (#254) (Sakib Hasan) * Allow non-id serial properties (#198) (zbarbuto) * Revert PR #246 (#248) (Sakib Hasan) * Add loopback-connector as peer dependencies (#246) (Russ Tyndall) * Fix operations on ended transactions (zbarbuto) * dbdefaults: Cleanup InvalidDefault def after test (Kevin Delisle) * Reuse the data source to avoid too many clients (Raymond Feng)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants