Skip to content

[SEMVER-MAJOR] Remove "loopback.autoAttach()" - #1989

Merged
bajtos merged 5 commits into
masterfrom
feature/remove-auto-attach
May 3, 2016
Merged

[SEMVER-MAJOR] Remove "loopback.autoAttach()"#1989
bajtos merged 5 commits into
masterfrom
feature/remove-auto-attach

Conversation

@bajtos

@bajtos bajtos commented Jan 25, 2016

Copy link
Copy Markdown
Member

The method was deprecated since LoopBack 2.0, there is no need to keep it around in 3.0.

Close #294

/to @ritch please review

@bajtos bajtos added this to the #Epic: LoopBack 3.0 milestone Jan 25, 2016
@bajtos bajtos added the #review label Jan 25, 2016
@bajtos
bajtos force-pushed the feature/remove-auto-attach branch from fed3c2d to 9e940ac Compare January 25, 2016 09:38
ritch
ritch reviewed Jan 26, 2016
View reviewed changes
Comment thread lib/registry.js Outdated

@ritch ritch Jan 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should assert or throw... How would the app function with this setting?

@bajtos bajtos Jan 29, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The setting will be ignored. If there are any models depending on autoAttach, then these models will return an error whenever a DAO method is invoked, something like "The model is not attached to any datasource".

On the second thought, I agree with you it's better to throw an error here, so that the problem is detected as soon as possible (at startup). Will rework.

@ritch

ritch commented Jan 26, 2016

Copy link
Copy Markdown
Member

Made a few comments. LGTM either way. Feel free to merge as is.

@bajtos bajtos assigned bajtos and unassigned ritch Jan 28, 2016
@bajtos

bajtos commented Jan 29, 2016

Copy link
Copy Markdown
Member Author

Uff, this change is much more tricky than I initially thought. The unit-tests are depending on autoAttach a lot. I'd like to clean up that as part of this PR, but it requires quite some effort. I'll post another comment when I make progress.

@bajtos bajtos added #wip and removed #review labels Feb 3, 2016
@bajtos
bajtos force-pushed the feature/remove-auto-attach branch 2 times, most recently from 3ed3af0 to da3991f Compare May 2, 2016 13:20
@bajtos

bajtos commented May 2, 2016

Copy link
Copy Markdown
Member Author

I have cleaned up the tests in 2fad45c and ff75613. Along the way, I discovered two issues:

  1. role.isOwner did not support app-local model registry, this is fixed by 6b699b9
  2. One of the tests was passing only because User model was already attached to a datasource, which may not have been the intention (it's hard to tell). I have fixed the test to pass without autoAttach to keep this patch smaller, we can discuss what's the correct behaviour outside of this PR. See da3991f#diff-bef842b910c6b8becd88271f314e19f6R343

@ritch could you please review again?

@bajtos bajtos assigned ritch and unassigned bajtos May 2, 2016
@bajtos bajtos added the #review label May 2, 2016
@bajtos

bajtos commented May 2, 2016

Copy link
Copy Markdown
Member Author

@ritch Also my intention is to back-port all commits except the last one (Remove "loopback.autoAttach()") to 2.x, as they are backwards-compatible. Any objections?

/cc @superkhau @raymondfeng

@superkhau

superkhau commented May 2, 2016

Copy link
Copy Markdown
Contributor

LGTM

@ritch

ritch commented May 2, 2016

Copy link
Copy Markdown
Member

LGTM as well.

@ritch Also my intention is to back-port all commits except the last one (Remove "loopback.autoAttach()") to 2.x, as they are backwards-compatible. Any objections?

Go for it 👍

@ritch ritch assigned bajtos and unassigned ritch May 2, 2016
@bajtos

bajtos commented May 3, 2016

Copy link
Copy Markdown
Member Author

@slnode test please

bajtos and others added 5 commits May 3, 2016 14:01
@bajtos
test/user: use local registry
095dce0 
Rework User tests to not depend on `app.autoAttach()` and global shared
registry of Models. Instead, each tests creates a fresh app instance
with a new in-memory datasource and a new set of Models.
@bajtos
Fix role.isOwner to support app-local registry
35d9fa4
@bajtos
test/rest.middleware: use local registry
0e6db30 
Rework tests in `test/rest.middleware.test.js` to not depend
on `app.autoAttach()` and global shared registry of Models. Instead,
each tests creates a fresh app instance with a new in-memory datasource
and a new set of Models.
@bajtos
Remove "loopback.autoAttach()"
87bbf45 
The method was deprecated since LoopBack 2.0, there is no need to keep
it around in 3.0.
@bajtos
Disable DEBUG output for eslint on Jenkins CI
9b39a59
@bajtos
bajtos force-pushed the feature/remove-auto-attach branch from 957db84 to 9b39a59 Compare May 3, 2016 12:02
@bajtos

bajtos commented May 3, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

For posterity, I had to make few tweaks to get CI passing on all machines: 8dc9c7e, edda89b and 957db84. I have also added an unrelated change 9b39a59 which I needed to be able to troubleshoot the CI failures.

@bajtos

bajtos commented May 3, 2016

Copy link
Copy Markdown
Member Author

@slnode test please

@bajtos
bajtos merged commit 7bc303b into master May 3, 2016
@bajtos
bajtos deleted the feature/remove-auto-attach branch May 3, 2016 12:35
@bajtos bajtos mentioned this pull request May 3, 2016
Merged
davidcheung pushed a commit to strongloop/loopback-boot that referenced this pull request Sep 22, 2016
David Cheung
Remove defaultForType option from tests
a044736 
this option has been deprecated since 2.x
tests already explicitly attach to datasources
autoattach was removed in strongloop/loopback/pull/1989
davidcheung pushed a commit to strongloop/loopback-boot that referenced this pull request Sep 22, 2016
David Cheung
Remove defaultForType option from tests
a02a730 
this option has been deprecated since 2.x
tests already explicitly attach to datasources
autoattach was removed in strongloop/loopback#1989
@davidcheung davidcheung mentioned this pull request Sep 22, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@bajtos bajtos

Labels

None yet

Projects

None yet

Milestone

#Epic: LoopBack 3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bajtos @ritch @superkhau