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

[CHORE]: Eliminate default injections in favor of re-export #6450

Merged
merged 9 commits into from
Sep 18, 2019

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Sep 14, 2019

  • eliminate explicit registration
  • re-export

@runspired Do we also need this?

ref #6166

@runspired
Copy link
Contributor

@runspired Do we also need this?

yes but for the adapter package

@snewcomer snewcomer changed the title Sn/re export adapter [CHORE]: Eliminate default injections in favor of re-export Sep 14, 2019
@Gaurav0
Copy link
Contributor

Gaurav0 commented Sep 16, 2019

Is there an RFC for this or is it mentioned in another RFC?

I think we need to deprecate access to the explicit registration but keep explicit registration working until 4.0, there are many applications that depend on it.

@runspired
Copy link
Contributor

@Gaurav0

I think we need to deprecate access to the explicit registration but keep explicit registration working until 4.0, there are many applications that depend on it.

Registration of these is an intimate API at most. We'll do the same thing here we did for the deprecation of the explicit serializer registration and target 3.17 removal https://github.com/emberjs/data/blob/master/packages/store/addon/-private/system/core-store.ts#L3314

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

There is no current -rest or -default adapter (-rest did exist recently but was never accessible in userland so we were able to drop is already). We shouldn't add them back nor auto-register them.

Copy link
Member

rwjblue commented Sep 18, 2019

FWIW, I was confused by the title here. When I saw "default injections" I thought that this was talking about the default injection of this.store in routes / controllers / etc. After reviewing this is about removing default registrations (not injections), and relying on the resolver to find these objects instead. I figured I'd comment here just in case others were equally confused (and don't take the time to review the diff).

Overall 👍👍 from me on this change...

@runspired
Copy link
Contributor

@rwjblue agree, but the naming is my fault I worded it this way in the project-trim task list. I have a bad habit of using registration and injection interchangeably due to how most of the time in initializers we were calling "inject" :(

Copy link
Member

rwjblue commented Sep 18, 2019

Oh, ya, no worries here at all, I was just confused until I read the diff (which looks great).

@runspired runspired merged commit b3d5da4 into emberjs:master Sep 18, 2019
@runspired runspired added the 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166 label Sep 18, 2019
@snewcomer snewcomer deleted the sn/re-export-adapter branch June 21, 2020 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants