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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(di): property injection with resolve #1748

Merged
merged 10 commits into from Apr 23, 2023
Merged

Conversation

bigopon
Copy link
Member

@bigopon bigopon commented Apr 22, 2023

馃摉 Description

Resolves #1742

馃帿 Issues

Add an export resolve to support the use case described in #1742. So now it's simpler to do this

Before:

abstract class Base {
  constructor(obj1: SomeType, obj2: SomeType2) {}
}

@inject(SomeType, SomeType2)
class MyElement extends Base {
  constructor(obj1: SomeType, obj2: SomeType2) {
    super(obj1, obj2)
  }
}

After:

import { resolve } from 'aurelia';

abstract class Base {
  obj1 = resolve(SomeType);
  obj2 = resolve(SomeType2);
}

class MyElement extends Base {}

It's also possible to call resolve with multiple keys at once:

const [abc, all_def_instances, someOther] = resolve(Abc, all(Def), optional(SomeOtherClass));

Doc is up at https://docs.aurelia.io/getting-to-know-aurelia/dependency-injection-di#property-injection

@bigopon bigopon changed the title feat(injected): field based di injection feat(resolve): field based di injection Apr 22, 2023
@bigopon bigopon changed the title feat(resolve): field based di injection feat(di): property injection with resolve Apr 22, 2023
@bigopon bigopon merged commit a22826a into master Apr 23, 2023
5 of 6 checks passed
@bigopon bigopon deleted the feat/field-di-injected branch April 23, 2023 09:22
AureliaEffect pushed a commit that referenced this pull request Apr 27, 2023
2.0.0-beta.5 (2023-04-27)

**Features:**

* **observer-locator:** ability to create getter based observer (#1750) ([ba40b2d](ba40b2d))
* **effect:** add watch ([ba40b2d](ba40b2d))
* **di:** property injection with `resolve` (#1748) ([a22826a](a22826a))
* **aurelia:** ability to inject with `Aurelia` export beside `IAurelia` ([a22826a](a22826a))

**Bug Fixes:**

* **plugin-conventions:** ensure esm cjs compat (#1751) ([f808503](f808503))
* **compat-v1:** dont use both writable and getter/setter ([b58f967](b58f967))

**Refactorings:**

* **build:** preserve pure annotation for better tree shaking (#1745) ([0bc5cd6](0bc5cd6))
* **router-lite:** alias registrations (#1741) ([f5e7140](f5e7140))
Copy link
Contributor

@Sayan751 Sayan751 left a comment

Choose a reason for hiding this comment

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

Great work @bigopon! Just a bit curious about all those test* functions.

}

/* eslint-disable @typescript-eslint/no-unused-vars, @typescript-eslint/ban-ts-comment, prefer-const */
function testResolve() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required? Or just an overlooked cleanup? If this is really a test, consider moving it to the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to test the typing of our exports without having to have a dedicated place for it. Example is around the built-in resolver: all/lazy/factory. The reason is because they need to give the right resolved types when called with container.get, and resolve, as well as not yield any errors when used as decorators. These functions will be eliminated during the build as well. This is a good and simple way to have TS check on these complex types.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's alright. I am not objecting to it. Rather, suggesting pushing it to tests, if the existing tests do not sufficiently cover this. And if they do cover, then these test* methods can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a bit hard to test the api in the test project. Since this is just API design test, not runtime behavior test, having the test code next to the code is really beneficial. We have this in multiple decorator files. Though if we want to organise differently, one way we could do is to have separate test files, but right inside this project rather than test, so it doesnt pollute the normal code.

packages/kernel/src/di.ts Show resolved Hide resolved
packages/kernel/src/di.ts Show resolved Hide resolved
packages/kernel/src/di.ts Show resolved Hide resolved
packages/runtime-html/src/dom.ts Show resolved Hide resolved
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.

RFC - Field level injection via added get function to the inject
2 participants