refactor(typings): added new column generic

[zewa666]

Alright this is fantastic, but at the same time requires a feature from current TS nightly. So I wouldn't deem that rdy to merge right now.

Anyways, open up Example1.ts which I've modified as sample (it's just a draft) and try to use intellisense on:

• field property of a column in the definitions array
• the formatters item
• and I'm sure it would work for dataContext of asyncPostRenderer as well

EDIT:
don't forget to check out the dot notation

Intellisense in action

[codecov]

Codecov Report

Merging #442 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #442   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          163       163           
  Lines        10233     10233           
  Branches      3484      3484           
=========================================
  Hits         10233     10233           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7934ff...4a4980a. Read the comment docs.

[ghiscoding]

quite a feat, can that be string | Join<PathsToStringProps<T>, '.'> or it's unnecessary? What does it show in the intellisense? I wouldn't mind seeing print screen of that in the PR

[zewa666]

Not next to a PC currently but if you Union with string this one wins since it includes all the Options from the other. In intellisense it shows the available dot.notations for T.

This one would certainly need more testing which is why i've created the draft so that you can fire your expectations at it. E.g can also a whole complex object be the field or only primitives? Not sure If thats currently handled.

Take your time as mentioned it relies on a next feature version of TS so there is plenty time to look at this ;)

[ghiscoding]

@zewa666
So I took a quick look at the PR and couple of comments

1. I see you added Generics to the Formatter, so isn't the intellisense going to work and show up the list of properties without the PathsToStringProps? I mean, I tried it with field: string and seems to work just the same, so I don't understand what it adds exactly. I re-read the note you first wrote and now I understand the field property of a column in the definitions array ... that is very very nice, I love it, that adds some much more type-safe.
   • Side note, I already added the Formatter: <T = any> in Slickgrid-Universal here but just forgot to do the same in Aurelia-Slickgrid
2. It would be better to use a different Example, perhaps Example 2: Formatter since I'd like to keep the Example 1 as simple as possible to show user how to get started with the easiest grid.
3. Something I wonder about the TS version that is used in my lib, is that going to require every user to use the same minimal version as I have in my lib for them to get going? Probably not but I need to make sure before using the latest and greatest TS feature since not everyone will use the same TS version.
   • I know TS version 4.1 goes RC on Oct. 30 and release on Nov. 12, so we might have to wait a bit for this PR, unless we use a Dev version as you made the PR with, that'd the be the first time ever that I use and test a Dev version.
4. That is very nice to see it working without passing the data interface for the inline Formatter. I guess that can't be done with an external Formatter because of the <T> that becomes required but still nice nonetheless.
5. Also note that a field containing the dot notation will automatically be treated as a complex object by the lib (you still have to provide the Formatters.complex but the everything else, that is the Editors, Filters and Sort will automatically treat the cell as a complex object). I'm saying this because I saw the Join<PathsToStringProps<T>, '.'>, so I wonder if that will have give a conflict? I guess it won't after I saw the demo with foo.bar.
   • I'm not sure what you mean by this question you wrote

     E.g can also a whole complex object be the field or only primitives?

   • What I can say though is that SlickGrid (core lib) always expect this to be a string (only) of the data property, while in Aurelia-Slickgrid I added a bit more to it by allowing a dot notation to be treated as a complex object (still a string but we explode it when a dot is found in the string).

Looks very promising, perhaps you could even post your answer to my SO question that I've referenced earlier since this goes even further than my original question.

I added some print screens to the PR description on top

[zewa666]

1. yep the T= any is there in the formatter definition but in not for the formatter property as there T is already initialized with any from the Column interface, so pretty much the same effects.

2. As mentioned this is a draft so yeah of course we'd pick a better example. Ideally we'd construct a new sample which uses both inline and standalone formatters + asyncPostRenderer to show the intellisense effects. Name wise it could be something along the lines of 30 - Advanced usage with TypeScript that would explain all the things related to this and other TS benefits.

3. yep it would definitely force users to switch to the latest TS versions in order to consume the library as lower ones would break with TS issues. There is a thing called typesVersions but that is more about specifying different targets as a consumer.

4. as for the standalone formatter you need to provide the generic to have the same effects but then can leave out the typings by yourself

5. what I meant with complex object is the following. Given your first screenshot, you see that foo.bar is an available option but not foo itself. Can foo be bound as well for the field?

[ghiscoding]

Thanks for the all the info, so I'd go back on the last 2 points then

4. I don't want to create a major (breaking) version, but I guess the minimum thing to do would be to, at least, wait for the final TS 4.1 version before merging this PR, so that would fall next month Nov. 12
   • I would rather do a major only after starting to use Slickgrid-Universal (common code) within Aurelia-Slickgrid and that is many months away.
5. Yeah there might be some odd, but probably rare, cases where some users would possibly want foo. I haven't tried but going back to what I wrote earlier, would it work to use field?: string | Join<PathsToStringProps<T>, '.'> or will that cancel the intellisense?
   • but I wonder if we really have to support that though, I mean technically speaking a Custom Formatter can use any other object property (through dataContext object), so even I write foo.bar it doesn't mean that I have to use it, so perhaps we could skip the support for foo... actually that is not true for Filtering/Sorting which do care about the field but even then we can use other options to override that with queryField, queryFieldFilter and queryFieldSorter.
   • Would you have a way to support foo or it's just not possible? Personally though, I don't think I ever used a grid that would use foo instead of foo.bar.

[zewa666]

4. Sounds ok to me
5. String will win in this case and negate all effects. I personally wouldnt support it either since worst case the user can cast whatever value is provided with any to make the error disappear. Moreover If the requests come in this can be fixed later.

How about the idea with the dedicated example? Or do you still prefer that its part of Example2?

[ghiscoding]

If you think an Example would make more sense and there's enough content to justify a new Example then go for it.

[zewa666]

yeah it wasn't essentially worth the hassle of a new example since it's not too much to show. I've added it now to Example2 as you proposed and reverted Example1. So for now as you suggested, let this rest for a while until the new TS version is released and than we can see how to go forward with this hone

[ghiscoding]

sounds good to me

[ghiscoding]

So I know there's still 2 weeks before the official release of TypeScript but I think 4.1.0 (RC) will be released tomorrow so we can start looking at this PR again. Going back to the question of "will it require the user to upgrade to latest version of TypeScript" and you mainly said Yes, then if that is the case, isn't better to add a peer dependency? Again I'm trying to avoid having to avoid releasing a breaking version and just go with a minor version.

I'm basically trying to delay a major until I'm switching to use the Slickgrid-Universal monorepo common code in Aurelia-Slickgrid. On that end, I wanted to do some cleanup and update all dependency and I'm nearly done, I made lot of these chore stuff in the last few PRs, I switched from TSLint to ESLint, upgrade from WebPack 3 to 4 (not 5 it's too new)... However there is still 1 more thing which you might be the best person to talk to. So my lib still uses "aurelia-i18n": "^2.3.2" but I didn't fully understand the changes from 2 to 3 (described here), is it just a setup changes that won't affect how I use the service in my lib? If so, does that mean I could just update the sample code and still be backward-compatible with all the users of my lib still using I18N 2.x? In my lib the I18N setup is here, if there's any new method names or if any got renamed then I guess I won't be able to upgrade but I think that is not the case... if you have time to create a PR, that would be fantastic.

[zewa666]

Peer dependencies wont help since you ship the d.ts file containing the new typings which will make older TS versions throw. Essentially you'd still enforce the user to update. Sad but true I don't really see any way instead of a major bump. You could argue that JS consumers wouldnt care but an automated build for TS users would most likely break, hence breaking change.

There is one more issue. Angular has only very specific Support for TS versions so this change could lock them out until 4.1.0 isnt properly supported by ng.

Since its not such a huge game-changing feature I'd recommend waiting until you're fine with making the move to universal and then add it to the major bump.

With regards to i18n you should only have to update both aurelia-i18n and i18next. If you didnt use Rother df/nf or the mentioned Baseclass no troubles. I can give it a shot and create a new PR for it

[ghiscoding]

There is one more issue. Angular has only very specific Support for TS versions so this change could lock them out until 4.1.0 isnt properly supported by ng.

I actually wasn't thinking to push this to Angular for that reason (at least not until Slickgrid-Universal is used in that lib too), I wanted to make it available in Aurelia-Slickgrid when TypeScript 4.1.x is out and also replicate the code change into Slickgrid-Universal.

I think it will take me couple more months to get Slickgrid-Universal used by Aurelia-Slickgrid (at least I got all the chore cleanup done now so I should proceed soon with the next step to use the monorepo into the lib but yeah couple more months). I like your change so I wanted to push it soon but I didn't want a breaking change mostly because that could mean 2 breaking versions in a matter of months but I wouldn't want to wait for your change to be available.

With regards to i18n you should only have to update both aurelia-i18n and i18next. If you didnt use Rother df/nf or the mentioned Baseclass no troubles. I can give it a shot and create a new PR for it

I'm only using the I18N Service and a few subscribe to the i18n:locale:changed event name and here is a good example of that. After the change, would we change to something like "aurelia-i18n": ">=2.3.2" to make it compatible for everyone? That would totally help if you take a look at it, thanks

[zewa666]

hmm if you bump to 3.x branch of aurelia-i18n users still on 2.x and using mentioned breaking changes would still result in such a change. So also this one in that case would make sense to do during your next major bump. The important aspect is that you allowed to override the used i18n service here so users with a more recent version can still use it, which is what I'm doing as an example. So again also here no need for a hurried update.

Again same as with typescript, you really want to make sure to address those kind of changes with a major bump, as that is pretty much the only way to guarantee your users auto-builds not to break since rarely anybody takes a * or latest as the version hint.

The downside of major bumps is that you can't any longer ship simple fixes to older majors without having to support multiple major lines. So I get your intention on wanting to keep them away as long as possible. Since there isn't as much activity here for the aurelia version tbh I'd just wait it out and do it properly as mentioned with the next big major push. For everybody who really wants and needs to do it earlier, e.g me, I can always patch-package/workaround issues until the new release lands.

[ghiscoding]

Alright it looks like you're more in favor of waiting for a breaking version for those 2 reasons we talked about, so that will come after I use Slickgrid-Universal within Aurelia-Slickgrid, so probably after Christmas then. I will soon start the work since the cleanup is mainly done.

I also wonder if there will be any breaking changes for Aurelia2 with my lib, I don't think so but I didn't have time to try Au2 yet. Doesn't look like it will ship before mid next year anyway, so I shouldn't wait for that before releasing a major version.

Thanks for all the feedback, that helps and happy to hear you will start using it soon 😉
If your demo becomes public, I'd be interested to see it in action

Cheers and thanks again

[ghiscoding]

I haven't advertised this feature because it's not available in Aurelia-Slickgrid (only is in Slickgrid-Universal) but I added this new SlickGrid Composite Editor Modal window. It might interest you, so take a look at the live Example, the 4 buttons on the right is the new feature (it took me over a month to get it all to what I wanted, from a built-in modal window Create/Edit/MassUpdate/MassUpdateBySelection and we mainly use it for Mass Update in Salesforce (and it looks like they want me to add Clone line as well)... So anyway, that will come in only after merging Slickgrid-Universal into Aurelia-Slickgrid because that was too much code to go through.

[zewa666]

Frankly I'm looking forward to the slickgrid-universal merge much much more since then contributions will make sense across the board.

Also with regards to v2 yeah might take a while for the Alpha, but there shouldnt be too much Issues except for the postRenderer part which will be doable. Anyways of you're going to look for more info hop over to the Aurelia discord channel which is primarily about V2. Would also making chats easier instead of misusing PRs and Issues ;) https://discord.gg/RBtyM6u

[ghiscoding]

Now that TypeScript 4.1.2 is official, I'm adding it into Slickgrid-Universal and this is beautiful 🏆

However I found a possible problem with this approach, there are some grids that the user wants to add his custom column without following the interface, for example in my use I have a column for "Action" but it's not part of my interface (and shouldn't be), the only way that I see dealing with this would be to disable the linter but do you have other suggestions to deal with those?

Well actually, I have this other possible solution, that might be the only viable solution, thought?
columnDefinitions: Column<ReportItem & { action: string; }>[];

[zewa666]

Yep thats what I asked a few posts back. Ok I'll give it a thought, perhaps we can find something. Is this a usual case would you say?

[ghiscoding]

Hmm that would depend on use cases, I looked at all the Examples that I've made and there's only 3 out of 29 that have such extra columns, it's mostly related to a column for "Actions"

• Example #3 icons on the left to Edit/Delete
• Example #11 delete icon
• Example #24 cell menu (aka action menu)

Though you might have seen other extra columns like the Row Selections, Row Move and Row Detail which are displayed as the first column with an icon, but for these extra features are added internally by the lib so they shouldn't matter.

So all in all, we're basically looking at the most common use case being an "Actions" column and I'm ok with just adding the type like I wrote earlier
columnDefinitions: Column<ReportItem & { action: string; }>[];

or add a built-in ActionColumn interface and extend it, but I'm not sure that really simplifies it
columnDefinitions: Column<ReportItem extends ActionColumn>[];

Also worth to know that the Action column, like I shown in previous print screen, also require a Custom Formatter to display the button/icon or text for it.

[zewa666]

Oh ok so there is just that action column. Thought there are more of these extra predefined columns. Yeah so your proposal sounds great with & action: string.

[ghiscoding]

@zewa666
I started the work for the next major (breaking) version that will use Slickgrid-Universal monorepo. I just started with the Formatters and had to override 2 of them, it seems to be going well, I mostly delete code more than anything else which is awesome, here's the Draft PR #466 I started

Couple of questions for you

1. your PR's code is already merged in Slickgrid-Universal by this PR, so should we keep your PR (and issue) open? Probably not since it becomes redundant.
   • EDIT - actually what might be better is to merge your PR into the new branch version-next-universal since you updated an Example and the models folder isn't deleted yet, so you can merge your code there
2. In Slickgrid-Universal, I bundle with TS as commonJS, ES2015 and ES2020 but after looking at Aurelia 2 package.json, they seem to use all ESNext everywhere, what's your taught on this? Which TS build should I keep? Does that make any difference? ...and actually I wonder what I should keep in Aurelia-Slickgrid as well as Slickgrid-Universal? We no longer need to support IE11, so there's that but the only side note is that I use WebPack in Slickgrid-Universal to bundle in UMD so that it works in Salesforce and I can't break that, but it's WebPack so it should be ok to use newer TS bundler.

# aurelia
{
  "main": "dist/esnext/index.js",
  "module": "dist/esnext/index.js",
  "types": "dist/index.d.ts",
  "typings": "dist/index.d.ts",
}

# slickgrid-universal
{
  "main": "dist/commonjs/index.js",
  "module": "dist/es2015/index.js",
  "typings": "dist/commonjs/index.d.ts"
}

3. and finally if you have a few minutes to give the next version a try, there's the task of updating to latest Aurelia-I18N which is on the list of things to do, so if you have time that'd be super appreciated

Again the next version PR is #466

[zewa666]

Hey ho,

@1:
yes I think we can close this one. It served it's purpose ;)
I'll contribute my example to your new PR

@2:
I think that doesn't really matter. You should be fine with what you have.

@3:
Sure I will, hope to be able to do so next week. Then I'll also update the i18n part.

[ghiscoding]

@zewa666
That sounds great, I also just sent you a collaborator invite, it should make it easier for you to contribute to the other branch and project. Happy Friday 😉

...oh and I can confirm that user won't be able to use the code unless they have TypeScript 4.1.x, I got into that problem that I couldn't use the Column interface (which has your code) from Slickgrid-Universal and then I noticed my VSCode TS version was 4.0.5 and it started working after updating to 4.1.2, so yeah it's pretty much a breaking change.

[zewa666]

Yep so as mentioned for Aurelia its most likely gonna be ok but for Angular until their CLI supports it it could mean troubles. Definitely good if this is given at least another month or so until release

[ghiscoding]

At this point it's not on my radar, I'm starting the process of using the monorepo in Aurelia-Slickgrid because it's simply easier than Angular-Slickgrid, and there's a lot of reasons why it is easier here

• Angular is stuck at certain TS versions probably need to wait Angular 11 or 12 until they finally bump TS to 4.1.x
• Angular still uses TSLint even though it was deprecated a year ago but I read ESLint is coming down the road (not a huge problem though)
• Angular-Slickgrid has RxJS code for Http calls with Backend Services (OData/GraphQL) while Aurelia-Slickgrid doesn't (Slickgrid-Universal doesn't use RxJS either, just regular Promises/Async)
• Angular-Slickgrid uses RxJS Subjects for pub/sub while in Aurelia-Slickgrid it's regular pub/sub with event name & data (again Slickgrid-Universal is closer to Aurelia).

So for all of these reasons, I'm not in a hurry to start using the monorepo in Angular-Slickgrid, I rather start with Aurelia-Slickgrid because Aurelia follows the standards and I tried to do the same in Slickgrid-Universal, so it's just much easier to get it working with Slickgrid-Universal. Aurelia community will benefit first because of their standards which was always a benefit of Aurelia 🚀

Thanks for the input and reminder though 😉