Remove 'columns' and 'options' object replacement on initialisation

[6pac]

This will allow passed-in options and columns objects to be maintained and updated, rather than extending them from an empty object during initialisation, which reassigns the variable and breaks the link to the original object.

var options = {
  editable: true,
  enableAddRow: true,
  topPanelHeight: 25
};
grid = new Slick.Grid("#myGrid", dataView, columns, options);

Before this PR, options would no longer be connected to the options variable inside the grid. After, it will remain connected.

This could be a breaking change only if code depends on the options or columns objects remaining unchanged, for example if they are used as an 'original state' which is to be preserved and reused later.

For columns, the main reason for preservation of the original object would be showing/hiding. Since the introduction of the hidden column property, this approach (which is over-complex and fragile) is able to be simplified and the existing columns shown or hidden simply by changing the property and refreshing the grid. This means this it is only ever necessary to have a single copy of the columns.

There is a new option useLegacySettingObjectConfiguration which, if true, preserves the previous method of initialisation. This PR defaults it to false for the time being so we can Break Stuff during testing.

For options, there is a new updateOptions method which can be used in place of setOptions. It will trigger changes to the grid after externally updating properties of the options object, and can trigger an onUpdateOptions event, which is different to the onSetOptions event in that it only passes the current options object rather than the before and after versions.

The columns property already has an updateColumns method which was added for the same reason, to provide grid updates after external changes to column objects in the columns array.

@ghiscoding wanting your evaluation and approval of this PR.

[ghiscoding]

these nested if could be regrouped under a single if since there's only 1 execution logic underneath

[6pac]

true

[ghiscoding]

does this code really have to be commented out? isn't that going to break something?

[6pac]

I don't think so, I decided it was actually redundant, but I'd have to double check. We no longer have the before and after state of the options.

[ghiscoding]

this new function seems oddly similar to setOptions, do we really need 2 functions that are similar with similar names? What's the difference between these 2?

[6pac]

covered below

[ghiscoding]

This will allow passed-in options and columns objects to be maintained and updated, rather than extending them from an empty object during initialisation, which reassigns the variable and breaks the link to the original object.

var options = {
  editable: true,
  enableAddRow: true,
  topPanelHeight: 25
};
grid = new Slick.Grid("#myGrid", dataView, columns, options);

Before this PR, options would no longer be connected to the options variable inside the grid. After, it will remain connected.

I'm not sure I understand what that means, we have a lot of defaults options, are these defaults going to be skipped? setOptions allows us to change 1 or more options while keeping other options in place, so what's the difference with this new updateOptions? I don't fully understand the reasoning of this new function, can you maybe give a real example of what you're trying to fix?

If this is a breaking change then you should probably start working on the next branch that I'm on (which is currently feat/esbuild), I have already migrated slick.grid to TypeScript class, though I found a bug that I have to investigate with row caching. Every time you push new code, I have to resolve all merging conflict or in this case I might have to apply the code by hand since I'm not sure how git works when the file extension and folder location changes (see my open PR and actually don't merge it yet)

[6pac]

This is basically as suggested by #666, but I've also applied it to my fork.

Everything works exactly as it does currently, but the passed in references to options and the column objects are preserved.

Example of current way:

var options = {
  editable: true,
  enableAddRow: true,
  topPanelHeight: 25
};
grid = new Slick.Grid("#myGrid", dataView, columns, options);

grid.setOptions({ editable: false});

options.editable =  true;  // local var 'options' is not linked to the grid and this has no effect

Example of new way:

var options = {
  editable: true,
  enableAddRow: true,
  topPanelHeight: 25
};
grid = new Slick.Grid("#myGrid", dataView, columns, options);

options.editable =  false;  // local var 'options' is the current options object
grid.updateOptions(); 

This makes very little difference in the above scenario, but for columns it makes a big difference. I generate all grid columns from metadata and create crossreferencing dictionaries (eg. column by name) before grid itinitialisation, but because all the column objects are recreated when extending from defaults any outside references to the elements of the columns array break:

      m = columns[i] = utils.extend({}, columnDefaults, columns[i]);

With this PR, defaults are applied to the actual passed in columns object instead of creating a new one.

To preserve existing behaviour, you set option useLegacySettingObjectConfiguration.
As commented, it will only be breaking anyway if code assumes the object is not connected and can be re-used.

We could also choose to apply this change to only the column objects and not the options object. There is a certain logic to having the before and after state of the options explicitly available.
Perhaps updateOptions should be configureGridForUpdatedOptions. That's what it does.

I'm happy to deal with any issues around releases. Can wait and I'll apply it later to the new v5 release. I really just wanted to get feedback and see it the tests all pass (they do).

[ghiscoding]

options.editable = false; // local var 'options' is the current options object
grid.updateOptions();

seems like an update by pointer, does that keep it as a pointer when updated?

I'm still not sure that I understand what this PR does, I'm pretty sure that on my side in order to change the locale (current language), I use the fact that the columns are saved by references and I think that if there's no more references/pointer then that would break my code at least for the locale switching part. At least I know I tried deep cloning the columns in the past and that for sure broke my locale switching because it really needs to change value by references, if this PR kinda does what deep cloning would do then I don't think I would be able to use this new feature myself

[ghiscoding]

@6pac I updated my previous comment, it would be good if you have an answer to it. I also understand that you wanted to see if the tests passes but at the same time, I managed to fully migrate slick.grid and slick.dataview to TypeScript last night in PR #804 and because I converted to TypeScript and also because I moved all the files to src/ then I can no longer cherry pick commits since Git doesn't know how to reapply changes since it can no longer find the original file... basically any new PRs would have to be reapplied by hand on the new structure and so large PR changes should be done in the new structure, I am only left with Controls & Plugins to convert to TS and that shouldn't take long (probably be done by the weekend) and then I have few things to review for the release scripts since I know I have to modify it all for the new structure.

I provided all info and instructions about the new structure in #804

[6pac]

OK, I feel like this is an important change, so I'll run it down. Once you get it, you'll immediately understand exactly what this is.

I'm overjoyed that you've done the TS conversion, and I'm more than happy to hand-reapply this to the new codebase.

OK, so currently, we pass in an options object. In init(), the variable is extended from an empty object with the are applied to it:

    options = utils.extend(true, {}, defaults, options);        

This replaces the reference to the original object.
Note that options, the internal variable in Slickgrid, is a closure so is copied and maintained with the new object within Slickgrid. But the original options local variable in the page still points to the object that was passed into the constructor, not the new extended object.

This PR changes behaviour so that instead of replacing the options valriable, it simply adds the default properties to it (only where the property does not already exist). This means that the reference is not broken, and the original options object still remains pointing to the 'live' options object.

Other than the reference remaining unbroken, the PR changes nothing.
Note that the same behaviour is possible with the current version using:

var options = {
  editable: true,
  enableAddRow: true,
  topPanelHeight: 25
};
grid = new Slick.Grid("#myGrid", dataView, columns, options);

options  = grid.GetOptions(); // updates the local options var to reference the new internal options

This PR allows the final line to be omitted, which may seem trivial.
Its importance lies in the fact that external code might want to be able to maintain a reference to the 'live' object. The final line does not update any external references to the original object referenced by the local options variable, and we are faced with having to update it manually in the external code.

It seems easier in principle to just have a single options object that can be referenced everywhere and has continuity over the whole grid lifecycle.

This is a significant, if very subtle, change in behaviour, and it is possible it might not be wanted. For example, if someone maintains a single options object and uses it to initialise multiple separate grids, they might want it to be left unchanged. This is a pretty easy problem for them to solve.

The updateOptions event is for when an option is changed and the grid needs to be updated to allow for the change. It executes the same code as setOptions but without actually setting the options object at any point. An additional constraint is that in the event we no longer have the 'before' and 'after' state of the object, only the current state. I'm not 100% happy with that, but I can't really think of a workable solution.

The PR also does the same thing with the elements of the columns array, so as to preserve external references.
I consider the change for the columns array to be more important than for options, and would accept just changing that.

The useLegacySettingObjectConfiguration option toggles between the new and old behaviours.

[ghiscoding]

I'm overjoyed that you've done the TS conversion, and I'm more than happy to hand-reapply this to the new codebase.

Yeah that was challenging to convert, mostly because it's better to migrate all these JS functions to TS Classes (also because we use them as constructor fn anyway) but I had many scope issues and had to use a lot of .bind(this) to make sure to use the correct scope (also because var doesn't have any scope, the only scope it really is inside the iife, so it was confusing sometime to know which function scope it was associated to). At least now it's a class, so it will be easier to understand the scope and maintain the code in the future

This replaces the reference to the original object.
Note that options, the internal variable in Slickgrid, is a closure so is copied and maintained with the new object within Slickgrid. But the original options local variable in the page still points to the object that was passed into the constructor, not the new extended object.

ahh ok now I understand, also note that now that the code is converted to TypeScript, we see the difference a bit more since the assignment is now to this._options instead of just options which was confusing considering it's the same name as the argument and the local function variable.

The useLegacySettingObjectConfiguration option toggles between the new and old behaviours.

hmm I don't really like this option name though, I would prefer to see the actual behavior in the name instead of seeing legacy, and we're also starting to have too many of these legacy stuffs. So maybe something along spreadOptionConfig (like ...myObj or extendOptionConfigObject (like Object.assign) or something similar which represent the new behavior... and by the way, why are we reassigning by looping, why not use spread or assign? wouldn't that do the same since we are assigning by reference in this case? Also note that Utils.extend(true, ... is for deep copy (true as first argument means deep copy), if you omit the true then it no longer creates a deep copy

[6pac]

I only put in useLegacySettingObjectConfiguration to remain non-breaking. If we're happy to make it a breaking change, we could omit this option altogether.

As I've said, it's only really the case where code depends on the passed in variable not being changed that is breaking, and the majority of the time the passed in var is a 'burner' that's only used to initialise and is then discarded. I think it's as easy to caution people to use

grid = new Slick.Grid("#myGrid", dataView, columns, utils.extend(true, {}, options));

if they want the old behaviour as it is to get them to use the flag.

You're right about using spead or assign, I've just been stuck in ES5 for so long (supporting IE11) that I'm really not up with ES6. I read up on the new features, think 'wow, that's cool' and then instantly forget all about them :-/

[ghiscoding]

I would prefer to keep both available but I just don't like to see legacy everywhere (for example I keep using the legacy autosize since I have my own resizer to combine it with), so having a name representing the behavior made more sense to me.

You're right about using spead or assign, I've just been stuck in ES5 for so long (supporting IE11) that I'm really not up with ES6. I read up on the new features, think 'wow, that's cool' and then instantly forget all about them :-/

Yeah the good thing about bundler is that you can write modern code ES2023 and tell the bundle what format we want output (ES6, ES2015, ...)

Also since I've merged my PR #804 on next branch with new structure, it might be a good idea to recreate/rebase this PR on the next branch

Side note

I also found out today that even though I thought our current version (v4) is supported by IE11, but it's actually not because I just found out that IE11 doesn't support template string literals (as shown on Can I Use), that is when we use ``` backtick (we started using some of them). I'm not sure if we should push fixes, by replacing them with string concatenation and push another patch release, I could also backport the large dataset scrolling throttling that I discussed yesterday.

[6pac]

Does the bundler replace the backticks if we select an old version of ES?
I suppose we really should remove the backticks in the current js version. Just looked, there's a few of them!!

[ghiscoding]

There's no bundler in current version, it's UglifyJS which just compresses code, there's no target to ES5 or anything of such. On the other hand with the new structure with esbuild, that is a real bundler but even then esbuild doesn't support ES5 target, it's minimum is ES2015 which is close to ES6 but without backtick conversion, IE11 is dead anyway (it's EOL since last year) so I am not interested in supporting it for our upcoming v5 but yeah we could fix the code for current v4 version, we just need to replace backtick with concatenation, the funny thing is that no one ever complained and we might have added some since v2 or v3

So when will you give a try to the new structure? You can checkout the next branch and get started 😉 I only have plugins left to convert to TS and I think I'll be done by next week, there's a lot of other tasks to do after though (like trying it in my libs) so there's still few more weeks of work but TS conversion is soon to be completed

[6pac]

Looks like you've already got started, but if that's the case with the backticks, perhaps we should make the executive decision not to support ES2015/IE11 in the latest release. I have been supporting IE11 for years, but just the last 18 months my clients have finally moved on. Perhaps the time has come ;-)

[6pac]

Also, I've noted before that I have a tool that you can set up a bunch of regexes in a text file and then it will apply them to every file in a folder structure, with extension filtering. It might be possible to set up our own automated conversion of the backticks pretty easily if we do want to support ES2015.

[ghiscoding]

Also, I've noted before that I have a tool that you can set up a bunch of regexes in a text file and then it will apply them to every file in a folder structure, with extension filtering. It might be possible to set up our own automated conversion of the backticks pretty easily if we do want to support ES2015.

Not that simple, I had to do multiple commits because backtick cannot be replaced by simple string concatenation, I had issues with scope, meaning that I had to wrap the code in brackets in a few areas while this wasn't required when using string literals, basically it's not that simple and the PR is done now... so up to you if you want to go ahead or not. I do not want to support IE11 in v5 but that is in the future, however for v4 I don't care that much and I'll leave it up to you if you want to do another patch version or not.

[ghiscoding]

@6pac so I closed the PR since we shouldn't care about IE anymore, it's been EOL since June of last year.

On another note, could you please think about rebasing your PR against the next branch, SlickGrid has been fully migrated to TypeScript and I only have a couple of plugins left to convert. I created this Discussion #807 with a Roadmap with a list of TODOs, you can update the list as you see fit

[6pac]

will do. just got back from holidays and busy now, but will try to rebase on the weekend

[ghiscoding]

hope the vacations was good 🍹

I now finished converting all files to TypeScript 🚀 Now the next step for me is to modify my libs to test them with these new files and do a lot of testing. So there's still a plenty of other things to do but it's coming along, see #807 for more info, cheers :)

[6pac]

OK, I'm onto a rewrite of this against next. It's a bit of a step function moving to TypeScript. What editor/env are you using to edit/test the project? I've got to choose one so may as well match what you use...

EDIT: perhaps a bit unclear, but what I'm asking also is how you compile the TS to JS for testing. Using node? Is this integrated into a 'build' shortcut with the editor?

[ghiscoding]

OK, I'm onto a rewrite of this against next. It's a bit of a step function moving to TypeScript. What editor/env are you using to edit/test the project? I've got to choose one so may as well match what you use...

EDIT: perhaps a bit unclear, but what I'm asking also is how you compile the TS to JS for testing. Using node? Is this integrated into a 'build' shortcut with the editor?

Hello Ben @6pac, so the best editor is of course Visual Studio Code, it has built-in TypeScript support so it will be easy to get started with TypeScript and you will love the intellisense that we now get with TypeScript :). I've put all the TS interfaces inside src/models/, most interfaces came from my Slickgrid-Universal project since I used TS from the start, I already had a lot of interfaces created.

I wrote a detailed explanation of the new structure and the installation in the first major PR I made on the next branch, I would suggest you read what I wrote in this PR #804 and if you have more questions then feel free to ask, there's basically 2 steps to get started with the new structure, npm install and then npm run dev which will start the dev server in watch mode and auto-recompile (using esbuild) anytime a TS file changes, like I said there's much more detail in #804 ... oh and make sure that you also have NodeJS 16 or higher installed. Also worth to know that the TypeScript code is now all in src/ folder (the code you can edit) and anytime a TS file changes, it will rebuild all the dist/ folder by itself (you never have to touch the dist folder, it's auto-generated, it will also display some info in the console because I added a bunch of console log to show what is happening)

I would suggest to close this PR and instead create a branch from next since there's a lot of code change and Git is not able to see the changes because we changed the folder location (src/) and the file extension (.ts), so for that reason it's better to start from the TS code on the next branch directly

Hey while installing VSCode, you should also install at the minimum the ESLint extension (click on the little puzzle icon on the side to reveal the marketplace) so that you get intellisense for any ESLint error directly in VSCode. I'm saying this because I actually just created a PR to install TypeScript-ESLint