fix: pasting multiple new rows

[zewa666]

I guess this feature was actually never properly tested but when pasting new rows at the bottom, with the defined newRowCreator, the previously auto created empty rows were not having an individual unique id, causing the subsequent actions to fail.

This changes it so that the burden is on the dev to provide the actual new row addition. At least that way he/she can control the newly created ids.

If you see this ok I would actually also update the docs to desribe the newRowCreator behavior.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[zewa666]

oh and btw I did also find one more place where the composite editor selection went wrong due to missing quotes

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.7%. Comparing base (f9db700) to head (e5863b9).
Report is 8 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1662     +/-   ##
========================================
- Coverage    99.8%   99.7%   -0.0%     
========================================
  Files         199     199             
  Lines       21893   21895      +2     
  Branches     7318    7035    -283     
========================================
- Hits        21831   21827      -4     
- Misses         62      68      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ghiscoding]

On the Composite Editor docs when adding a new item, I went with 2 possible approaches

• user can optionally provide his own id via insertNewId option
• user can omit the id, if so the id will be dataset length + 1

this.compositeEditorInstance?.openDetails({
      headerTitle: modalTitle,
      modalType,
      // insertNewId: 1234, // you can provide a custom Id (defaults to last Id+1)
      // insertOptions: { position: 'bottom' }, // if you wish to add the item to the bottom (defaults to top of the grid)
      onError: (error) => alert(error.message), // you should define how to deal with error coming from the modal

      // you can optionally provide an async callback method when dealing with a backend server
      onSave: (formValues, selection, dataContext) => {
        // simulate a backend server call which returns true (successful) after 30sec
        return new Promise(resolve => setTimeout(() => resolve(true), 500));
      }
    });

I never use this feature, so you're probably best to think what the correct approach is but still I think we can do the same as what I've done in Composite for that one.

On my side, I started working on migrating from Jest to Vitest, I still have 270 that are failing (out of 5000) which I need to debug, so that will keep me busy for a while. I think Jest is faster than Vitest on Windows however, but still I want to drop CJS on next major (probably won't be until next year though)

[zewa666]

oh thats great news with regards to Vitest, let me know if I can help.

the idea with the default length+1 does not really make sense as the user is supposed to provide a newRowCreator which should handle the job. so the code as was would actually fail bc it would do a double addition. As for id generation I think its kinda dangerous to assume defaults as the ids might already be jagged due to deletes, and in combo with softdeletes create issues.

[ghiscoding]

ah well ok, like I said, I don't use this feature so much, so go ahead with what you think is best. But for the id generation, I think the dataset length + 1 is still an ok approach by default and if that is not correct for some then just use the optional insertNewId which seems fine for me. Anyway if the newRowCreator is required then it's another story.

[zewa666]

but how would I infer that the newRowCreator function wasnt creating rows and than doing it as fallback with length+1? please take another look at the diff, I hope from the conditional surrounding the change it should clarify what I meant

[ghiscoding]

but if you remove this, then how will the item be added to the dataset? You're forcing the user to do that part itself, is that it? isn't this a breaking change though?

[zewa666]

it is but the old behavior didnt work either as it clashed with existing ids, more so it actually created elements with an undefined id.

and yes, I'd expect the user to do it by himself as thats actually the only safe way to handle it imo

[ghiscoding]

can we somehow add some console warning in the case that the user forget to add it?

[ghiscoding]

@zewa666 I've opened my PR for the Vitest migration (it's not fully done yet, it has coverage decrease which I have to look into) and since it's touching every test files, it will certainly conflict with your PR. So I'd like to merge your PR before Vitest but would it be possible to add a console warning like I mentioned just above?

[zewa666]

sure will try to do so today

[zewa666]

took a bit longer but now it's in. Was it something like this you meant?

[ghiscoding]

and yes, I'd expect the user to do it by himself as thats actually the only safe way to handle it imo

The console warning I asked for was mainly for what you wrote above, if the user forgets to do it himself, then show them a warning that they didn't. I'm not sure if your new console covers this or not, you know more than I do on that part.

Side note, this new console will be missing test coverage, you could also ignore it with Istanbul (or v8 for upcoming Vitest), though if you can add a unit test then use something like below for spying on the console

slickgrid-universal/packages/utils/src/__tests__/domUtils.spec.ts

Lines 90 to 93 in e97017a

	const consoleWarnSpy = jest.spyOn(global.console, 'warn').mockReturnValue();
	createDomElement('div', { className: 'red bold', innerHTML: '<input />' });
	expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining(`[Slickgrid-Universal] For better CSP (Content Security Policy) support, do not use "innerHTML" directly in "createDomElement('div', { innerHTML: 'some html'})"`));

[zewa666]

I guess thats the only way to check that, cant make any assumptions about how the resulting ids should look like.

yep will update the tests. thx for the sample console check

[ghiscoding]

looks good, merging :)

The next step will be to update & merge my other Vitest migation PR (probably tonight).
@zewa666 I assume that issue #1618 is still out of the loop for now, right?

[zewa666]

yep feature got postponed. first conditional columns and footer rows are on my plate ;)

we can close the PR as well if you prefer and I'll reopen once back at it

[ghiscoding]

@zewa666 that's fine, I just like to have updates on the status once in a while, sorry to ask too often 😉

BTW, I got some small questions about Vitest, but I'll ask them in the Vitest PR #1663

first conditional columns and footer rows are on my plate ;)

Side note, SlickGrid actually has a slick-footer section and the only place I saw it used is in the Total Rows (which I added recently in Example 24). I'm not sure what else we could do with this footer, but basically it follows the same column width and cell height, so that might be useful for you to know.