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

Bug: Cannot use spread with the injector (Injection With Inheritance in Aurelia) #171

Closed
silbinarywolf opened this Issue Nov 19, 2018 · 19 comments

Comments

Projects
None yet
3 participants
@silbinarywolf
Copy link
Contributor

silbinarywolf commented Nov 19, 2018

I'm submitting a bug report

  • Library Version:
    1.4.1
    (Webpack build uses native-modules dist)

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    v8.11.1

  • Yarn Version:
    1.5.1

  • Webpack Version
    Webpack 4.16.5

  • Browser:
    Chrome

  • Language:
    TypeScript 3.0.1

Current behavior:
Using the spread operator doesn't work as per the blog post here with TypeScript, for some reason it seems to generate an additional empty object parameter?

I also have to make each property optional on the base class, which isn't ideal but workable.

https://ilikekillnerds.com/2016/11/injection-inheritance-aurelia/

import { autoinject, containerless } from 'aurelia-framework';
import { I18N } from 'aurelia-i18n';

@autoinject
@containerless
export abstract class BaseDialog {
	constructor(
		protected readonly controller?: DialogController,
		protected readonly element?: Element,
	) {

	}
}

@autoinject
export class NotesDialog extends BaseDialog {
	constructor(
		protected readonly i18n: I18N,
		protected readonly dialogUtils: DialogUtils,
		...rest
	) {
		super(...rest);
		// rest[0] = {}; <- Why is this empty object here?
		// rest[1] = DialogController
		// rest[2] = Element;
		console.warn('NotesDialog', rest);
	}
}

Additionally, if I console.log the "NotesDialog.inject" static property at run-time, I get:

0: ƒ I18N(ea, signaler)
1: ƒ DialogUtils(dialogService, i18n)
2: ƒ Object()

Expected/desired behavior:
I'd expect to be able to use the "...rest" pattern so I don't need to update each subclass if I update the dependencies on the base class.

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Nov 20, 2018

@EisenbergEffect I might be able to devote some time to fixing this issue, are you able to provide any insight on where I should start or how this could/should be resolved?

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Nov 20, 2018

@silbinarywolf Just to establish a common understanding first: are you expecting the dependencies from the base class to be appended to the dependency list in the subclass? e.g. [I18N, DialogUtils, DialogController, Element]?

Important note:
Nothing can be done about the last Object parameter. This is a limitation in TypeScript which has sort of been addressed in the latest version (you can use generic type params for those) but I doubt the metadata generation has been addressed accordingly. In short, that ...rest parameter has no type for typescript's perspective, and typescript can only make an object of it.

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Nov 20, 2018

I should also point out that the blog post you're referring to is using regular @inject - the key difference is that it doesn't need typescript's reflection in order to infer the types. That's why this works with @inject but not @autoinject.

That said, I think it's possible with some tweaks to get the type lists merged at a metadata level.

The area where you'll want to be looking at if you want to help is the autoinject decorator in injection.js. You would need to traverse up the prototype chain and merge the inject from the base class into the subclass.

Not sure if this can be done without causing regressions, but it's certainly an idea worth exploring

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Nov 21, 2018

You're correct, I would want dependencies from the base class to be append, as per your example.

[I18N, DialogUtils, DialogController, Element]?

I assumed that the 'Object' was metadata was something on the TypeScript side but I wasn't 100%, so this is good to know!

I'll see if I can spare some time and see how possible this is :)

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Nov 21, 2018

I assumed that the 'Object' was metadata was something on the TypeScript side but I wasn't 100%, so this is good to know!

This happens to be a thing I ran into when working on DI in vNext as well. When you don't specify the type (or invalid, like an interface) typescript simply emits the Object type. Frankly i'm a little surprised it does the same thing for ...rest but I suppose it makes sense to someone somewhere :)

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Nov 21, 2018

@fkleuver I'm not convinced the TS team has devoted adequate time to thinking about how a number of these scenarios work. I think they are still waiting for the new decorators spec before their willing to go further and flesh this all the way out.

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Nov 21, 2018

@EisenbergEffect Yeah I believe so too. Shouldn't change the normal metadata emit though. With one special check in place to just ignore that object, ...rest should still be possible if only you merge the metadata from the prototype chain. Not sure why we weren't already doing that tbh.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Nov 21, 2018

@silbinarywolf Would you like to work on this still? It sounds like @fkleuver has an improvement in mind.

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Nov 27, 2018

@EisenbergEffect I'm gonna make some time for this within the next 2 weeks. However if @fkleuver wants the issue, just ping at me here and I'll make sure to not start on anything.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Nov 27, 2018

I think you can probably go for it!

@fkleuver

This comment has been minimized.

Copy link
Member

fkleuver commented Nov 27, 2018

Be my guest @silbinarywolf , feel free to ping me if you run into any roadblocks of course :)

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Nov 27, 2018

Will do. Cheers!

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Dec 10, 2018

Starting to look into this today

silbinarywolf pushed a commit to silbinarywolf/dependency-injection that referenced this issue Dec 11, 2018

silbinarywolf added a commit to silbinarywolf/dependency-injection that referenced this issue Dec 11, 2018

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Dec 11, 2018

I've got a PR up now

silbinarywolf added a commit to silbinarywolf/dependency-injection that referenced this issue Dec 13, 2018

silbinarywolf added a commit to silbinarywolf/dependency-injection that referenced this issue Dec 13, 2018

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Dec 17, 2018

@EisenbergEffect Are you able to give an ETA on the NPM release? Just so I can roughly know when I should defer one of my work items to. (It's dependent on my recent fix)

(Apologies if you'd rather a new Github issue, but that felt a little heavy for what I was asking!)

https://www.npmjs.com/package/aurelia-dependency-injection

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 18, 2018

I'll attempt to get it out this evening.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 18, 2018

Released on NPM :) Thanks for helping out!

@silbinarywolf

This comment has been minimized.

Copy link
Contributor Author

silbinarywolf commented Dec 18, 2018

Thanks for the quick turn-around in putting out a release!

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 18, 2018

Glad to help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment