feat(editor): add dependency injection support in editors

[jmzagorski]

The current state supports editor dependencies through the Column.params property. By adding Factory.of, with aurelia's container, we are wrapping the creation of an editor in a function that already resolves injected dependencies at the time the grid is bound. Factory.of returns a function that passes any number of arguments to the editor. When Slickgrid calls currentEditor = new useEditor({}), the function created by Factory.of passes the slick grid arguments to the editor's constructor after the injected dependencies, so order matters.

• Update Custom Editors Wiki to assert that DI is now supported but order of dependencies does matter
• Update Localization Wiki

-Passing i18n in the Grid Options for Formatter or Editors
+Passing i18n in the Grid Options for Formatter

Also I changed the compoundDateFilter file that was not using the injected i18n and using the older params.i18n logic. Hopefully that is okay.

closes #25

[ghiscoding]

you removed this function, but what happen if the user didn't do a full setup i18n? I can't remember what is the minimal even if you have only 1 locale to support, I think that I required to at least setup i18n with 1 locale. You might be in a better situation to confirm since you use only 1 locale, right? I just want to make sure that there will always be a locale returned on line 100

[jmzagorski]

oh yeah I meant to ask you about that. I guess when I tested it I forgot that you probably set it up in your demo? I never used this library so I thought that since it returned "en" whenI removed that function that the library is smart enough to return the default language

[ghiscoding]

if you aren't using i18n then what does that return? If nothing is found, it should always return 'en'

[jmzagorski]

yeah, if it i18n is not setup in main.ts, the currentLocale will return undefined. the new commit should fix it

[ghiscoding]

could you add a simple comment on top of the for loop to briefly describe what the loop does. This makes it easier for anyone to jump in the code at any time.

[ghiscoding]

Great, tested it locally and everything works fine. In the demo, you can change the locale by going in Example 12 and switch to French then come back to both Example 3-4 and see the French text appearing in both MultipleSelect and DatePicker. Good job.

I guess the only other place I currently use the i18n is in the TranslateFormatter. However Formatters are returning string, can that still be done for them too? I guess not since they are simple function returning string, but you might have an answer on that too.

[jmzagorski]

I thought about the formatters too. To get DI into a formatter it has to turn into a class. Then when you bind it to the column.formatter property you still want to bind a simple function because formatters should be simple/fast. So I think it is up to the user to either put the dependency on the params property or they could inject the formatter into their view model if it has a lot of dependencies. For example (i have not tested this)

@inject(MyFormatter)
export class Example {
   constructor(private: myFormatter) {
    this.columns = [{
      formatter: myFormatter.format.bind(formatter) // the format function is something i made up
    }]
  }
}

[ghiscoding]

Were does the this.columns come from? Is that the column definition?

[jmzagorski]

yes, this.columns is the view models column definitions that are passed to your grid. I made it a very very simple example.

[ghiscoding]

Ok so this is really at the App level (where the grid is created).

1. I guess it's still easier to pass the params: { i18n: this.i18n } to the gridOptions for the formatter: translate. However the demo you wrote could be interesting, as you said earlier, for Formatters that needs multiple dependencies (in that case, we could simply update the Wiki - Custom Formatter with your example.

2. For version 2.x, if we want to use Formatter with classes instead. I think that would not be possible to use formatter from the gridOptions since that is a SlickGrid property and it does expect a function returning a string. Not sure if SSR (Server Side Rendering) could help in this situation!?

3. This makes me think that I eventually want to change all my Services and remove any init() functions. I'd like to use a Shared Service that I can pass the (grid, dataView, gridOptions, gridDefinitions, ...) whenever they get created and then in all Services, just use regular DI with that Shared Service.

Thoughts on any of these? I'd like to know if number 3 makes sense but I slightly started that concept in my Angular repo for the Grouping feature. So I'd like to know if it's best approach before going too far with it. For example, this line would be removed and in that Service it would simply use DI on the Shared Service (which would already have the grid object, I think). I just hope the DI in these services won't try to read the grid object too early but I don't think that would happen.

[ghiscoding]

@jmzagorski
Do you mind sharing your thoughts on the last post I wrote, especially number 3.
Thanks

[jmzagorski]

for Formatters that needs multiple dependencies (in that case, we could simply update the Wiki - Custom Formatter with your example.

I can test out my example to see if it works before updating the documentation so we can show another way to use formatters. I will put a TODO item on the contributing/doc issue so I remember to do it and it does not get lost in this closed pull request.

For version 2.x, if we want to use Formatter with classes instead. I think that would not be possible to use formatter from the gridOptions since that is a SlickGrid property and it does expect a function returning a string. Not sure if SSR (Server Side Rendering) could help in this situation!?

I think for now we can leave the formatters because they function well, and I am not familiar enough with SSR yet.

I'd like to use a Shared Service that I can pass the (grid, dataView, gridOptions, gridDefinitions, ...) whenever they get created and then in all Services, just use regular DI with that Shared Service.

I do not think we can get DI to work with shared services that pass runtime objects such as the grid, dataview, gridOptions etc. The reason for this is looking at the init function in the grid services. init is doing the work that DI would be doing because it is similar to a constructor call to new up a service by passing in all the dependencies needed for the service to work. If init was not called, the service would be broken because it is not in the right state. Creating a singleton stateful service has some drawbacks, and we discussed one of those yesterday with multiple grids on the same page. I can think of 3 options if you wanted to change your services, both of which would fix the multiple grid problem because it would not have one state.

1. Inject all grid services as transient. This means every slickgrid custom element will get an new instance of the service and work with just with that grid. The downside of this is that these services cannot be used in view models since a new instance is created every time it is injected. However, it would fix the multiple grids on a page issue

2. Use an object on all services to keep track of which grids are passed into init by either using the object itself or the grid id. This could get complicated because every method call to a service would have to pass some key so the service knows which grid to operaton on. Since you would make to pass in a key anyway as an additional argument for this to work, number 3 would be a better choice.

3. Make a stateless services. By no state I mean they do not have properties for the grid or anything the custom element created at runtime, they just perform a service on the grid, dataview, columns or whatever is passed into their methods. For example this line would pass the grid and row number as parameters. Users can still use your service in their view models because you expose grid objects anyway and if a method required the grid they could pass it. This would allow multiple grids to be rendered on the same page too.

After typing all of that, number 1 would be the least amount of change, but break use of your services outside of the custom element. I would not recommend number 2. Number 3 is another good option because services should be stateless, but would required a rewrite of all service methods requiring a grid object.

Hopefully this all makes sense lol.

[ghiscoding]

Thanks for this review, it's really great that you took the time to write such a detail response.

You seem to like number 3 a bit more than 1, however that requires user to change their Service method call to add the extra grid object. Also how would they get this grid object? ..through the onGridCreated Event Aggregator, or did you have something else in mind? I'm always worried about breaking changes, and they should be reserved for version 2.x so I'm not sure how to go forward with this, and I also would like to get multiple grids on same page to work.

[jmzagorski]

Yes, number 3 would have to be in version 2 and most likely they get the grid from some event or leave it exposed as a bindable.

I think number 1 is just as good and didn't want to rule it out. It potentially would not add any breaking changes while fixing the multiple grid problem (i think). We would need to think of a way to expose your service methods with 1 or many grids, which is challenging if we inject the services with @transient. Based on the index file that would be quite a few service methods:

[ghiscoding]

Would you mind giving a shot at it?
I'll be doing the Grouping in the coming days

[jmzagorski]

Yes, I will mess around with different ways.

[ghiscoding]

@jmzagorski
I released new version 1.11.0 over lunch which include this PR.

Did you have any Wiki update to do?
Just trying to make sure everything is covered. Thanks

[jmzagorski]

Yes the wiki is updated under Custom Editors in https://github.com/ghiscoding/aurelia-slickgrid/wiki/Editors.

Great job on the release 👍