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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Installed 14.2 but having performance and display issue not seen with 13.0, also had to convert to ES5 #10878

Open
pinuchfab opened this issue Mar 27, 2024 · 16 comments

Comments

@pinuchfab
Copy link

pinuchfab commented Mar 27, 2024

Describe the bug

We upgraded to 13 then 14.2 as trial, from 13.0 onward handsontable.js is now ES6, we bundle all our js into a single js using requirejs, the spread ES6 operator is not supported and we get an error when bundling. I finally managed to use Babel to revert the ES6 code to ES5 and bundling worked for both v13 and v14, strangely enough, v13 is performing well and display well, but v14 simply is slower in term of performance when scrolling up or down, this is using Eco Rendering, also the body of the table is not aligned with the header and we have one extra row that contains th elements... In other words, we currently cannot use 14.2, we have made no code change on our side and no errors is thrown in the browser. See picture attached.

Video/Screenshots

image

Provide a link to the demo with the bug reproduction

No response

Handsontable version

14.2

Framework version

No response

Your environment

Version 122.0.6261.129 (Official Build) (64-bit)

@pinuchfab pinuchfab added the bug label Mar 27, 2024
@AMBudnik
Copy link
Contributor

Hi @pinuchfab

We do our own performance testing with each version to make sure that there is no degradation. It may happen for some plugins' sets that we do not use. But I would need to have a demo to replicate the issue (especially as you mentioned eco-renderers that we do not have).

@pinuchfab
Copy link
Author

Hello, thanks for quick reply, I have asked the team to use 12.4 as this is the most reliable release so far post 8.1, I had to make a couple of small changes but overall I am happy with it. I will setup a demo after the holiday, I just wanted to start the chat really. Another problem I found is that once the columns are set using updateSettings, then if columns are moved using the column move plugin, it is then not possible to set the original columns back with updateSettings anymore, even so the right order of columns is passed, this was working in 8.1, not working anymore in 12.4, not a major problem, but we would need to find a workaround. I might log a separate defect for this.

@AMBudnik
Copy link
Contributor

If you like the v12.4.0 version, then I think that there is something particular in the settings/plugins that is set to be slower in v13. We have reported that there is a performance drop for merge cells, but that happened for the v12.0 (versus the older version). So, without a demo it will be hard to say anything here. Still, please take your time and enjoy the holiday.

When it comes to the manualColumnMove, I would need to create a step-by-step scenario to replicate the issue. And in 14.2.0 we even added an ability to undo the move action for columns (via this PR #10746).

I will keep this issue open. Please contact me after the holiday.

@pinuchfab
Copy link
Author

pinuchfab commented Apr 16, 2024

Hello, I am now back.
So here are the steps to try on your side using 12.4.0:
-> create a table with 10 columns ([ A, B, C, D .. ] as an example)
-> have a button on the UI that when clicked on it will run that code:
btnClickCb = () => {

          const newColumnsToSetInHot = [ A, B, C, D .. ];  // Essentially reconfigure columns as they were before the move.
          myHotInstance.updateSettings({ columns: newColumnsToSetInHot }, false); // true or false makes no difference.

}
-> manually move second column to third position (drag)
so we will end up with [ A, C, B, D, ...] shown in the UI.
-> click on button to reset columns order to be [ A, B, C, D, ...]

Expected: [ A, B, C, D ]
Results: [ A, C, B, D ]

Obviously this is a simplification of the code, but this behavior used to work with 8.1.

On another note, the business has asked us in the case we stick with 12.4.0, which we want to as 14.0 was not good for us, how long do you plan to support 12.4.0 in case we have security issues being identified, even so we have not had any. Finally, are you aware of any security issue related to 12.4.0.

Thanks so much for your help.

@pinuchfab
Copy link
Author

pinuchfab commented Apr 16, 2024

I would like to mention we don't find any values (for our business) to undoing or redoing column dragging, we are only using undo/redo for cell/row modifications (editing vs preferences), not for columns moving, which will actually be a problem as we consider columns ordering as part of user's preference that should not be mixed with editing, this could actually be really frustrating for a user to lose their columns ordering when they undo or redo their changes if they did change that preference between two edits of a cell for instance... I hope this is optional. Thanks.

@pinuchfab
Copy link
Author

pinuchfab commented Apr 16, 2024

Latest: I have written an example and there are some odd behaviors that can be observed when moving columns, title of the column is not propagated after a drag. We find this is a very unreliable plugin and to be honnest it was also not working great in 8.1.

https://codesandbox.io/p/sandbox/handsontable-12-4-0-demo-auto-updating-formulas-forked-w48fgq?file=%2Fsrc%2Findex.js%3A144%2C1

@AMBudnik
Copy link
Contributor

Hey @pinuchfab

how long do you plan to support 12.4.0 in case we have security issues being identified, even so we have not had any. Finally, are you aware of any security issue related to 12.4.0.

In general, there are no security issues with the following version if we can believe what SNYK knows. However, if we find anything, we still make changes to the latest version.

Latest: I have written an example and there are some odd behaviors that can be observed when moving columns, title of the column is not propagated after a drag.

To have the column names moved they need to be defined. Setting up colHEaders to true is not enough. In this case I recommend using title.

Here https://codesandbox.io/p/sandbox/handsontable-12-4-0-demo-auto-updating-formulas-forked-l4llv7?file=%2Fsrc%2Findex.js%3A32%2C31 is an updated example. Now the title is given per column in columns configuration so it follows the column when they are reordered.

@pinuchfab
Copy link
Author

Ok I am going to take a look at your example.
Thanks for replying in regards to security and maintenance.
I suppose, as I am just experiencing, you don't mind supporting older versions such as 12.4.0 ? As one of the main reason we cannot move to 14.0+, as well as the problems with performance, is the lack of a version generated for ES5, traditionally speaking we do get multiple bundles to choose from, amd, commonjs and so on, an ES5 missing version starting at 13.0 was a bit of a harsh discovery for us, as a lot of legacy products cannot afford (for many reasons I cannot enumerate) switching to ES6, it will take a lot of time. Any chance you will be proposing a ES5 bundle of future versions or have you really stop supporting ES5 all together?

@pinuchfab
Copy link
Author

pinuchfab commented Apr 18, 2024

Ok I have tried your demo following the link, when a drag let s say B after D, it works, when I click the button then the table gets reset accordingly, but then when I drag B again after D ... it doesn't work, B title stays in place.

So I changed the code in the callback to have columns with name : { { name: ... }
This worked, but then when I drag columns again the width gets lost and all column eventually have the same width, this is really strange, it feels like there is some state kept after a drag that applies the width..
I m going to check in our code, but I am pretty sure we keep the title as well on resetting back to original columns setup as this is the same code run at initialisation.

Follow up:
I have check our code and the problem still persists, I believe I have identified what could be the cause, I think updateSettings doesn't like if there is a mixbag of properties to set, I have noticed that if I only pass columns it works, but if I pass something else then the columns property is ignored... I m trying to pin point what is the extra settings that could cause the problem at the moment and I will be able to update the defect a bit once I know.

@pinuchfab
Copy link
Author

pinuchfab commented Apr 18, 2024

Please try this fork:
https://codesandbox.io/p/sandbox/handsontable-12-4-0-demo-auto-updating-formulas-forked-p8lkpq?file=%2Fsrc%2Findex.js%3A137%2C34

If for some valid reason, we want to update both the data and the columns via updateSettings, then after a drag column operation, we get some very odd results in the columns. This is on top of the other problems I found related to the dragging after a reset without any data changes.

@AMBudnik
Copy link
Contributor

Could you please tell me what are the replication steps for the new demo https://codesandbox.io/p/sandbox/handsontable-12-4-0-demo-auto-updating-formulas-forked-p8lkpq?file=%2Fsrc%2Findex.js?

@pinuchfab
Copy link
Author

pinuchfab commented Apr 22, 2024

The steps are as follow:
-> create a table with 10 columns ([ A, B, C, D .. ] as an example)
-> have a button on the UI that when clicked on it will run that code:
btnClickCb = () => {

      const newColumnsToSetInHot = [ A, B, C, D .. ];  // Essentially reconfigure columns as they were before the move.
      myHotInstance.updateSettings({ **data: [a collection of rows]**, columns: newColumnsToSetInHot }, false); // true or false makes no difference.

}
-> manually move second column to third position (drag)
so we will end up with [ A, C, B, D, ...] shown in the UI.
-> click on button to reset columns order to be [ A, B, C, D, ...]

Expected: [ A, B, C, D ]
Results: [ A, C, B, D ]

The only difference from my original demo, is that the use case if we want to restore original columns order as well as updating the data, this is useful for instance when a user wants to reset the table as it was originally sometimes with fresher data from the back end.

@AMBudnik
Copy link
Contributor

AMBudnik commented Apr 22, 2024

Thank you for sharing the steps to replicate the issue.
I was able to get the described result.

It seems that using data within the updateSettings() is causing the columns to remain in the old position. If you comment that out reset works well.

I replicated the same issue on a simpler demo and simpler environment as well. I will investigate this case and come back to you once I get a bit more details.

@AMBudnik
Copy link
Contributor

@pinuchfab Based on the details at https://handsontable.com/docs/javascript-data-grid/api/core/#updatesettings, when you use updateSettings with data, it no longer resets the plugins, so the order is attached. You may, however, manually alter the order of columns to manualColumnMove: [0, 1, 2, 3, 4, etc.] to get them as they were before the move.

@pinuchfab
Copy link
Author

pinuchfab commented Apr 22, 2024

Ok thanks for the explanation, another work around I tried is to call updateSettings twice, once with just the columns related settings, then for the other cycle I only update the data, separating the calls into two cycles seems to do the trick as well.

In our opinion both solutions are workarounds. What I would suggest is that the core of updateSettings apply the changes as well as update the internal rows, columns and plugins mappings like before (8.0?), then the onAfterUpdateSettings callback is called so that we can trigger our business logic once the table has finished maintaining its internal states.

I have noticed the new loadData / updateData functions.. Does that mean that we can do something like:
hotinst.updateSettings(); // Any settings except data?
hotinst.loadData([...]); // Feed some new data

?

We are very confused about the reasons of that change for 12.0, updateSettings was doing everything at once without us having to worry about what plugins will need to be updated and so on.

Re ES6, did you get any feedback about providing a ES5 version of the dist in the next release? As using babel to convert ES6 back to ES5 so that requirejs can bundle ok is not very elegant for us, we do maintain a big legacy product and upgrading our tooling just to bundle Handsontable will be regarded as costly and risky both in term of DEV and QA.

I will get back to you with more details on the performance issues we found when using 14.0.

I do apologize for bringing 3 different issues related to the upgrade, I know it is a lot, but this upgrade has been particularly difficult and we want it to be reliable going forward as we did have a very solid solution up to this point, we really want 14.0+, but it is not possible as yet.

@AMBudnik
Copy link
Contributor

If you would like the data change to work as before (reset the plugins) then just move to the loadData() method. That would be

  btn.addEventListener("click", () => {
    hotIns.loadData(initialDataset);
    hotIns.updateSettings(
      {
        //data: [...initialDataset],
        columns: [
          {
            data: "brand",
            title: "A",
          },
          {
            data: "model",
            title: "B",
          },
          {
            data: "priceWithoutVat",
            title: "C",
            type: "numeric",
            numericFormat: {
              pattern: "$0,0.00",
              culture: "en-US",
            },
          },
          {
            data: "priceWithVat",
            title: "D",
            type: "numeric",
            numericFormat: {
              pattern: "$0,0.00",
              culture: "en-US",
            },
          },
          {
            data: "productionDate",
            title: "E",
            type: "date",
          },
          {
            data: "daysFromProduction",
            title: "F",
            className: "htCenter",
          },
        ],
      },
      false
    );
  });

This change was requested by many users to keep the plugins as they are, so that became the default. That was also mentioned in the migration guide.

I will get back to you with more details on the performance issues we found when using 14.0.

Could we get back to that in another issue report or at [email protected]?

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

No branches or pull requests

2 participants