exportWithFormatter not respected when column has editor

[zewa666]

Hi there,

I've been tweaking my copy&paste feature where a couple of columns are foreign-key lookups.
That means I fetch data for the grid, which contains the master data, that references other tables for details. So some columns have a value being an id, wheras the label is a column from the references foreign key.

Master Data

name	mail	memberOf
Zewa	[email protected]	1

Resolved members

id	label
1	Admin
2	Writer
3	Reader

Now the fictional column memberOf has a custom formatter, which looks up the label of the members lookup source and formats the output. Additionally there is a custom editor, which is a select box that receives a lazy datasource, and pre-selects the value based on the provided id.

So the resulting rendering of the grid would be

Name	Email	Member
Zewa	[email protected]	Admin

If I now select all cells, copy and paste them into Excel, I'll get the result
Zewa \t [email protected] \t 1

So I've seen that columns can be annotated with exportWithFormatter but still the output is the same. After digging into the code I found here that the setting is only respected if the grid isn't editable or the cell does not have a defined editor.

Long story short, why is that defined this way?
Second question, would it perhaps be a good idea to add a global option that overrides the other two checks?

[ghiscoding]

Long story short, why is that defined this way?

Short answer... I have no idea hahaha, I didn't create this plugin (like many others), I simply copied it from SlickGrid which was in plain JS at the time and rewrote it in TypeScript. If it doesn't work the way you want, feel free to make changes. However I think that the goal was to make sure the cell doesn't have an open editor (so internally SlickGrid calls the makeActiveCellNormal to close the potentially opened editor). The thing is that whenever an Editor is opened, the way to read the cell value has to come from the Editor serializeValue() I think, so I'm guessing that to avoid any conflicts (like maybe a cell value change not yet save), it makes sure the cell is a regular cell.... hmm actually re-reading the line you referenced it points to a Formatter so I guess I'm going the wrong way with my explanation. It looks like it's missing the read to a regular cell, isn't it? If you think of a way to fix your issue, I would probably accept it.

slickgrid-universal/packages/common/src/core/slickGrid.ts

Lines 5263 to 5268 in 9223338

	/**
	* Make the cell normal again (for example after destroying cell editor),
	* we can also optionally refocus on the current active cell (again possibly after closing cell editor)
	* @param {Boolean} [refocusActiveCell]
	*/
	protected makeActiveCellNormal(refocusActiveCell = false) {

Second question, would it perhaps be a good idea to add a global option that overrides the other two checks?

Not exactly what you mean, maybe need more explanations on that.

---

Side note, I changed my focus to another dependency used by the project. I have started rewriting excel-builder.js to TypeScript. The "why" is because the project is so old and is written with ancient technologies like Q (the ancestor to Promise) and lodash which isn't a bad thing but since it's written in CommonJS (with require()), it does come with the entire libs. I thought of maybe using other Excel libs but as it turns out even XLSX (aka SheetJS) and other libs are still CJS project and Excel-Builder is much smaller since it only writes (doesn't have any read functions) which is what I want to SlickGrid....

So long story short (for me as well lol), I'm making good progress, I got it mostly all converted to TypeScript, dropped Q. I'll push the code soon, my next step is to remove all Lodash usage (if possible) and maybe also replace JSZip with fflate to download the Excel file. I'm assuming (hoping) for a large decrease in build size (that is also assuming the developer is actually using @slickgrid-universal/excel-export). Since I'm sticking with Excel-Builder, I'm expecting to simply push a minor release in here (though it will be a major in that project)

I'm going to rename the updated lib to excel-builder-vanilla (pretty much similar to the package rewrite I've done with multiple-select-vanilla). Once that is done, it is yet another package that we will be able to remove from Angular allowedCommonJsDependencies (excel-builder-webpacker) and also great for the future since eventually Vite 6 will remove CJS support and perhaps I might also migrate my project to ESM only some day maybe.

The only other large change I want to do in the future would be the switch from moment-mini to Temporal but it is taking the ECMScript committee forever to finalize their specs (it looks like this year is when that will finally happen though, I've been waiting 3 years for this to happen)

[ghiscoding]

BTW, you can take a look at the original plugin for comparison. One thing to note is that SlickGrid never had any exportCustomFormatter in its code, that was my own stuff and you won't find it SlickGrid which is why the code is slightly different

https://github.com/6pac/SlickGrid/blob/b0af2169ae9e8cddc07823cd583a4bb4be1d2c8f/plugins/slick.cellexternalcopymanager.js#L81-L108

[zewa666]

the open editor situation makes sense as you described. but in case I navigated via arrow keys or marked the cell as part of others, so no active editor, it would also skip the formatter. I think I'll try to modify the condition and see if everything still works. I'll add a PR for that.

thats great news about the excel plugin. dropping the need for cjs would be huge plus contributing changes/fixes to the TypeScript base would be way easier.

with regards to temporal have you thought about using a polyfill in the meantime? https://www.npmjs.com/package/@js-temporal/polyfill
once the commitee accepts the solution you'd merely have to drop the polyfill. pretty much what I did back than with aurelia i18n and intl.numberformat

[ghiscoding]

I did think about the polyfill, which is probably the only way to start with something new anyway, but I'm not sure I really wanted to start using it until the spec is more finalized and more Temporal code start to be implemented in major browsers. Anyway this is probably a large task and I've tagged that one as low priority, moment-mini as the name suggest is not as large as the original moment one because moment-mini doesn't include any locales which we don't need anyway.
There are still other CJS dependencies left, like Flatpickr & Autocompleter both of them are half kinda of ESM without being ESM, but at least I have worked on removing a lot of them. I might have to replace Flatpickr on the long run since there's barely any development there and their ESM build is not really ESM... anyway after 5 years on this project, I managed to have advanced my project quite a lot in the right direction, don't you think!? 😉 Especially for something that is mainly in my spare time

[zewa666]

cant say anything about flatpickr since I'm using ng-select for my custom editor but it could be quite complicated finding something new thats cross-framework safe. dunno moment-mini but I guess its similar to dayjs (6kb).

true, this lib came quite a long way back from when I remember you taking your first steps with it 😄

[ghiscoding]

I tried both date-fns and dayjs, both of them had issues and I think the biggest problem I had with both was the missing Z (Zulu) support, it wasn't working well with UTC dates with T...Z and I think it's a must for a datagrid that uses these kind of dates coming from DB. Hopefully this will be easier with Temporal and TimeZone stuff, at least the target is to obviously solve all these issues, so we'll have to wait and see.

This project is also my playground to test all kind of new technologies, I learned a lot from it haha. Things like Jest, Cypress, GitHub Actions, hybrid bundling for CJS/ESM... I learned a lot really. It's also nice to contributions like yours once in a while hehe 😉

[zewa666]

With dayjs UTC is available as additonal plugin https://day.js.org/docs/en/plugin/utc. Even with that it should be way lower than moment-mini which still ticks in at around 50kb

[zewa666]

so while trying to fix this I had an issue. I can't really only check for !this._grid.getCellEditor() since the editor could be opened somewhere else but not on the copied cells. Ok so instead I wanted to check the targeted row/cell and compare that with the activeCell, since thats the one if a editor is present.

But now I realized the function https://github.com/ghiscoding/slickgrid-universal/blob/master/packages/common/src/extensions/slickCellExcelCopyManager.ts#L151 actually doesn't care about the cell alltogether and only ever copies row/cell 0,0.

I can't really think of any reasoning why it's doing that so I'd assume this is just a dummy implementation that was kept around.

I think what should be done instead is to pass the row/cell here (https://github.com/ghiscoding/slickgrid-universal/blob/master/packages/common/src/extensions/slickCellExternalCopyManager.ts#L436) to getDataItemValueForColumn and from there on to the next step here https://github.com/ghiscoding/slickgrid-universal/blob/master/packages/common/src/extensions/slickCellExternalCopyManager.ts#L120 so that it's finally available in the original location to both fix the 0/0 bug as well as implement the proper conditional check

here's whats currently happening:
I've modified Example19 to have a custom formatter outputting the cells index and row. Additionally all columns have exportWithFormatter:true. So while I'm entering a value through the editor, I'd still expect the formatters output to be copied

[ghiscoding]

I can't really only check for !this._grid.getCellEditor() since the editor could be opened somewhere else but not on the copied cells.

You should probably use the SlickEditorLock for that, or the SlickGlobalEditorLock which is has instance of it globally available and then maybe cancel any left opened editors SlickGlobalEditorLock.cancelCurrentEdit() and I already see this being used directly in that plugin.

slickgrid-universal/packages/common/src/core/slickCore.ts

Lines 502 to 510 in c15d4f1

	/**
	* A locking helper to track the active edit controller and ensure that only a single controller
	* can be active at a time. This prevents a whole class of state and validation synchronization
	* issues. An edit controller (such as SlickGrid) can query if an active edit is in progress
	* and attempt a commit or cancel before proceeding.
	* @class EditorLock
	* @constructor
	*/
	export class SlickEditorLock {

I'm rarely, if ever, using this plugin so if you think it could be improved then go ahead for it. You also probably understand more than I do at this point, so...

[zewa666]

okay I'll give it a try and see what happens

[ghiscoding]

@zewa666
hey BTW, have you tried Biome instead of Prettier? I decided to give it a try because I'm a little mad at Prettier, they pushed a change 2-3 weeks ago that adds comma to all props in tsconfig.json and I hate that, at the beginning they said it wasn't a breaking change (even though it's obviously a breaking format change to most of us) and that there was nothing for them to fix, but I think they finally decided to make a change after so many people complained about now having invalid JSON files... anyway, I've installed Biome in my new excel-builder-vanilla it's pretty damn fast too since it's written in Rust (it was originally called Rome but they had licensing or ownership issue so they renamed it), I love it so far, it replaces Prettier and also ESLint, there's only 1 dep to install (Biome) with a little config file and you're good to go