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

feat: introduce devMode to support nodejs based unit testing #944

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Dec 6, 2023

as the title suggests this is meant to make unit/integration tests, e.g with testing-library, possible for apps using Slickgrid. In my specific case I'm making use of angular-slickgrid and would like to be able to test my app using angular-testing-library.

The introduced devMode workarounds are in order to work around the following jsdom issues:

For additional conversations about this topic please refer to ghiscoding/Angular-Slickgrid#1319

P.S.: if this PR is fine I can later create an additional one targeting the master branch

@zewa666
Copy link
Contributor Author

zewa666 commented Dec 6, 2023

instead of the containerClientWidth property on devMode, we could as well leverage the already existing options.options.viewportMinWidthPx. The only downside is that I'm not sure whether this would introduce potential issues with the autosizeColumns feature. (see function internalAutosizeColumns(autosizeMode, isInit)).

Moreover I'm pretty sure more jsdom issues will surface as I continue testing so having devMode as a collection of "workarounds" instead of a simple boolean flag is more forward-looking

@@ -2116,6 +2117,11 @@ if (typeof Slick === "undefined") {
var i;
if (!stylesheet) {
var sheets = (options.shadowRoot || document).styleSheets;

if (options.devMode?.ownerNodeIndex >= 0) {
Copy link
Collaborator

@ghiscoding ghiscoding Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zewa666 so just to be clear, the devMode can be of type boolean or { ownerNodeIndex?: number; containerClientWidth?: number; }, is that correct? I'm asking because I'll have to backport this to the SlickGrid v5 with TypeScript and also into the next major major Slickgrid-Universal code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly right. I can also do the backport to v5(master?!) as mentioned above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright I'm ok with the PR then as mentioned in paragraph below. So let me go ahead and merge the PR, I'll let you do the backport to v5, thanks for that.

I'll push new SlickGrid versions and other upstream libs probably in the weekend.
On all my upstream libs, I've now switched to GitHub Action workflows to publish releases because I wanted to publish on npm with --provenance and it's now much quicker to push releases, just hit start workflow :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds great. getting late over here so I'll open the PR for v5 tomorrow. thx

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 6, 2023

The Dev Mode seems like an acceptable approach for testing libraries, I think however that SlickGrid might also be used in a Node project (without a browser or maybe with Python, I don't really know) so the Dev Mode wouldn't be representative for that use case but that could be corrected in the future with another condition to the if I guess. But, I personally only ever use SlickGrid in a browser and I'm not sure how that could be used in a plain Node project (which uses global instead of window) and I guess that since we also have a few more window object usage across the project, there would be a bunch more errors to fix anyway... all that to say that I guess I'm ok with the Dev Mode approach since it doesn't introduce any breaking change and it does helps with testing libraries like Jest and and others

For anyone's reference, this PR came from a long Discussion in Angular-Slickgrid project and that project still uses SlickGrid v4 hence why this PR is based on the version4 branch

@ghiscoding ghiscoding merged commit 40d2dc4 into 6pac:version4 Dec 6, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants