feat!: upgrade to Aurelia 2, closes #709 #1038
Conversation
|
@MaximBalaganskiy great thanks a lot for this PR, I've done a quick review without running the code yet. You could also give me access to write on your branch so that I could maybe fix some stuff myself within the same PR if possible (or I could merge and fix them later since it's on the Side note, we're working (mainly myself) on the next SlickGrid (core lib) as well and there's still more work to do on that project, I've converted that old JS code project to TypeScript and ES6/ESM (see our SlickGrid v5 Roadmap and I'll probably want to align the releases at the same time as the Au2 support since there are some breaking changes in the core lib as well (we'll no longer have the SlickGrid (core) on the anyway long story short, I assume there's still work until around Sept. (back to school lol) on that side of it as well. aurelia-slickgrid/src/examples/slickgrid/example3.ts Lines 631 to 634 in 85cee52 |
|
The access should be there. As for the timeframe, I'm personally not in a rush, I've got an Aurelia1 project which is not in production (might not even be released in the end) and upgrading it is not on my list atm. |
- add back `sass-loader` - add missing Flatpickr CSS - add Example 15
| @@ -108,13 +99,26 @@ export class AureliaSlickgridCustomElement { | |||
| private _isLocalGrid = true; | |||
| private _paginationOptions: Pagination | undefined; | |||
| private _registeredResources: ExternalResource[] = []; | |||
| private _columnDefinitionObserver: ICollectionObserver<CollectionKind.array>; | |||
| private _columnDefinitionsSubscriber: ICollectionSubscriber = { | |||
| handleCollectionChange: () => { | |||
There was a problem hiding this comment.
@MaximBalaganskiy
I'm back to working on this migration and found that this _columnDefinitionObserver only works once and not after, would you have any clue why that is?
to replicate the issue, you can get latest code and go to Example 3 then click on the "Dynamically Duplicate Title Column" button multiple times, it only add an extra column on the first time but not after and after debugging and adding a console log in here, I see that the subscriber only worked once
I should have wrote my comment on the subscribed line, it's basically the following lines of code (at line 597)
this._columnDefinitionObserver = this.observerLocator.getArrayObserver(this.columnDefinitions);
this._columnDefinitionObserver.subscribe(this._columnDefinitionsSubscriber);
|
I think replacing the array makes the subscription inactive. The first time
it works because you push first. Either do not replace the array or
resubscribe when it changes.
…On Fri, 15 Sep 2023 at 1:46 pm, Ghislain B. ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/aurelia-slickgrid/custom-elements/aurelia-slickgrid.ts
<#1038 (comment)>
:
> @@ -108,13 +99,26 @@ export class AureliaSlickgridCustomElement {
private _isLocalGrid = true;
private _paginationOptions: Pagination | undefined;
private _registeredResources: ExternalResource[] = [];
+ private _columnDefinitionObserver: ICollectionObserver<CollectionKind.array>;
+ private _columnDefinitionsSubscriber: ICollectionSubscriber = {
+ handleCollectionChange: () => {
@MaximBalaganskiy <https://github.com/MaximBalaganskiy>
I'm back to working on this migration and that this
_columnDefinitionObserver only works once and not after, would you have
any clue why that is?
to replicate the issue, you can get latest code and go to Example 3 then
click on the "Dynamically Duplicate Title Column" button multiple times, it
only add an extra column on the first time but not after and after
debugging and adding a console log in here, I see that the subscriber only
worked once
—
Reply to this email directly, view it on GitHub
<#1038 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4KNPHGVMRXNL4S7EHH3SLX2PFQRANCNFSM6AAAAAA3JGG64U>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I digged into Aurelia 2 docs and found that the columnDefinitionsChanged(newCols, oldCols) {
console.log('columns changed', newCols, oldCols);
}then the console log does get increase by another column every time I click the button. The only thing that I see from Au2 docs is that compare to Au1 it doesn't call the initial Changed event but we can call it ourselves from the
Well that goes against what the user were able to do in Au1 and I still want to support this use case anyway, users can and should still be able to change the column definitions at any point in time |
|
It was probably an oversight. I like to keep props and changed handlers
together - different to your code style - that is why I missed the
connection and deleted that method.
On Fri, 15 Sep 2023 at 2:08 pm, Ghislain B. ***@***.***>
wrote:
… I think replacing the array makes the subscription inactive. The first
time it works because you push first. Either do not replace the array or
resubscribe when it changes.
… <#m_-4248179730306974074_>
On Fri, 15 Sep 2023 at 1:46 pm, Ghislain B. *@*.*> wrote: @.** commented
on this pull request. --------------------
I digged into Aurelia 2 docs and found that the xChagned is still
supported (as per docs
<https://docs.aurelia.io/components/bindable-properties#calling-a-change-function-when-bindable-is-modified>)
and if I add back simple code like this
columnDefinitionsChanged(newCols, oldCols) {
console.log('columns changed', newCols, oldCols);}
then the console log does get increase by another column every time I
click the button. The only thing that I see from Au2 docs is that compare
to Au1 it doesn't call the initial Changed event but we can call it
ourselves from the bound() method... so was there any reason you decided
to go with the approach you had? Was it to make the code a bit cleaner
or...? Should I put back previous implementation with
columnDefinitionsChanged?
—
Reply to this email directly, view it on GitHub
<#1038 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4KNPARZG6324KPXENJX6TX2PIETANCNFSM6AAAAAA3JGG64U>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
ok no problem, I'll add it back since it worked better, I just might need to run it once for the initial load to make it the same as Au1 but at least it should make it work again :) Thanks for the info... and time to go to bed, it's passed midnight here :O |
|
@MaximBalaganskiy
I see that you migrate step 2 because of this Au2 migration BindingEngine has been removed in v2 comment but with the current I currently got it working by using both and re-subscribing the observer whenever aurelia-slickgrid/src/aurelia-slickgrid/custom-elements/aurelia-slickgrid.ts Lines 581 to 598 in a842b75 |
|
Not sure why they made it this way. I'm not a big expert in au2 so not
aware of other alternatives. I've found documentation is lacking with
regards to observation, or may be I'm blind :)
…On Sat, 16 Sep 2023 at 11:31 am, Ghislain B. ***@***.***> wrote:
@MaximBalaganskiy <https://github.com/MaximBalaganskiy>
Looking back at the code, it looks like I need both because just one
doesn't fulfill all use cases
1. columnDefinitionsChanged for assignment like this.columns =
newColumns[...]; or .slice()
2. getArrayObserver, which used to be bindingEngine.collectionObserver,
for bindings and array methods like .push, .pop and others...
I see that you migrate step 2 because of this Au2 migration BindingEngine
has been removed in v2
<https://docs.aurelia.io/developer-guides/migrating-to-aurelia-2#binding-engine>
comment but with the current bindingEngine.collectionObserver with Au1,
it was working fine and I only needed to create and subscribed to the
observer just once in the bind lifecycle and I was done, but it looks
like with Au2 the getArrayObserver really does need to be re-subscribed
like you mentioned earlier since it seems to lose its subscription somehow.
Do you know why that is? Do you know if there's something else that I can
use which can combine both assignment and array methods observing?
https://github.com/ghiscoding/aurelia-slickgrid/blob/a842b750e1ac074d1dfea64bbcd3c0b84c4d3069/src/aurelia-slickgrid/custom-elements/aurelia-slickgrid.ts#L581-L598
—
Reply to this email directly, view it on GitHub
<#1038 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4KNPCI7V4KVHUKBPB2XZ3X2T6QBANCNFSM6AAAAAA3JGG64U>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
- `columnDefinitionsChanged` binding is only good for assignment but we also need to watch for changes via array methods like (push, pop, ...) so for that we also need an array observer
Codecov Report
@@ Coverage Diff @@
## next #1038 +/- ##
===========================================
- Coverage 99.29% 22.80% -76.48%
===========================================
Files 15 8 -7
Lines 974 895 -79
Branches 331 310 -21
===========================================
- Hits 967 204 -763
- Misses 7 691 +684
|
|
@MaximBalaganskiy At first, I got this error
Do you have a working setup for a production build? I'm pretty sure that it has to work with my servor little server, if it doesn't then the GitHub demo page won't work either. Any chance you can help with this? For now, I'll use WebPack in dev mode to run CI E2E tests... Note that I also tried my Au1 WebPack config but that doesn't at all with Au2 |
|
The prod build only builds a plugin, what are you serving after it? |
@MaximBalaganskiy The build for publishing the lib on npm is a separate process, what I want to fix is the website build for the GitHub demo page and also for the CI |
ohhhh I see what you mean now, I was not using WebPack for the plugin build in Au1 but it looks you are in Au2, I did not noticed that. I was using TypeScript build previously, would that still work or should I really need to use WebPack for the plugin build? Anyway, here is what I modified in WebPack to get the website demo working as I wanted.
As I mentioned in previous paragraph, I was using simple TypeScript scripts to build the plugin, could I still use these scripts or should I really use WebPack to build the plugin? aurelia-slickgrid/package.json Lines 34 to 41 in a3567d6 |
|
I think it's time to merge since this PR is starting to be quite large already, however the Aurelia-Slickgrid component unit test is entirely missing, I seem to have problem running the unit tests for that one in particular but that can be added in the future (or skipped entirely, it doesn't make much difference on the usage since all Cypress E2E tests are passing). Please note however that a new major release is still a month or two away since there need to be a major restructure in the Slickgrid-Universal project that I need to work on next. Thanks a lot @MaximBalaganskiy for all of this, I would not have been able to migrate without your help, so thanks a lot!!! |
|
No problem, I wasn't expecting such a quick upgrade anyway. Nice to know that it will definitely happen now. |
|
Hey so I just wanted to give an update on the Aurelia 2 upgrade, I'm still working on the next major version of Slickgrid-Universal and it took me much longer than expected but anyway I'm pretty sure that I'll be done in the next couple weeks (before Christmas for sure), for more info on what is left to, you can take a look at the Slickgrid-Universal Roadmap 4.0. Cheers |
As discussed, copied the upgraded files keeping the original location.
I was a bit confused by the demo app structure, so I've modified it just enough that it compiles.
Non-converted TS files got "ts_old" extension so that compiler does not pick them.
One thing that got me during conversion is that in many examples the mock data method mutates the array AFTER it's already bound to an element. This results in weird grid issues. So the way to do it was to build an array first and then assign to the field.