ComboBox - New methods + bullet proofing #479

mohsinhijazee · 2024-10-10T17:17:29Z

This PR proposes few additions to the ComboBox.

I have tested this with SvelteKit and wrapped the Combox as a Svelte Component with full events and multiple instances of the component on the same page fetching data from difference sources so many copies/instances work seemlessly.

Few fixes/improvements:

Constructor does not rely on window.$hsComboBoxCollection being initialised. Thus users for bundlers (vite etc) don't have to load all of preline.js.
Fixed an error about invalid nullish statement
Fetching and rendering items is now bit more bullet proof, ensures that the returned response is iterable.

Additions to the CombBox:

apiSearchQueryTransformer to allow transforming query before fetch. This is required in case some APIs use more advanced filtering DSL such as Google's AIP-160 for example filter=prefix('name', 'sh') OR contains('lastName', 'sh') etc.
selectedItem() returns elemnent that was selected
selectedValue() returns the value.
selectedAttr(attr) returns the custom attribute.

Few other changes to the package.json (required dependencies added, deprecated type definitions replaced with updated packages), few scripts/watch/commands and generation scripts added.

Don't have full context on design choices so some things might seem absurd and I am here to revise as per your feedback FYI @jahaganiev and @olegpix

olegpix

@mohsinhijazee Hi!
Thank you for your work! Please read the comments left. Correct the PR according to the comments and push changes.
Best regards!

olegpix · 2024-10-23T15:18:42Z

pnpm-lock.yaml

Please remove this file from the PR. We don't use PNPM.

olegpix · 2024-10-23T15:24:14Z

package.json

Please focus on TS files (src/plugins/combobox/*.ts).
You can create a separate PR in which you will provide your vision of bundling. But again, we do not use PNPM.
Please remove this file from the PR.

I don't have clear idea on bundling, I thought that maybe there's some off line step that only maintainers have or similar. I'll remove them.

olegpix · 2024-10-23T15:25:58Z

index.d.ts

Please focus on TS files (src/plugins/combobox/*.ts). You should not generate output files on your side. Please remove this file from the PR.

olegpix · 2024-10-23T15:26:12Z

preline.d.ts

Please focus on TS files (src/plugins/combobox/*.ts). You should not generate output files on your side. Please remove this file from the PR.

olegpix · 2024-10-23T15:27:01Z

dist/combobox.d.ts

Please focus on TS files (src/plugins/combobox/*.ts). You should not generate output files on your side. Please remove this file from the PR.

olegpix · 2024-10-23T15:29:28Z

src/index.ts

We fixed this issue through another PR request from another user. Please remove this file from PR.

olegpix · 2024-10-23T15:31:29Z

src/plugins/select/index.ts

Please create a separate PR for the select. We will be grateful if you will separate PRs by making them separate for each plugin.

olegpix · 2024-10-23T15:34:31Z

src/utils/index.ts

Please do not use this as a separate helper. This check can be done with the standard Array method Array.isArray

Yes, that's kinda pointless. 😄

olegpix · 2024-10-23T15:35:01Z

src/plugins/combobox/index.ts

@@ -13,12 +13,14 @@ import {
 	afterTransition,
 	htmlToElement,
 	isParentOrElementHidden,
+	isArray,


Please use Array.isArray instead.

Makes sense. Will do.

olegpix · 2024-10-23T15:35:32Z

src/plugins/combobox/index.ts

@@ -376,6 +396,12 @@ class HSComboBox extends HSBasePlugin<IComboBoxOptions> implements IComboBox {
 	}

 	private jsonItemsRender(items: any) {
+
+		// Bullet proofing.
+		if (!isArray(items)) {


Please use Array.isArray instead.

Changes to the Combobox - Constructor does not rely on `window.$hsComboBoxCollection` being initalised. Thus users for bundlers (vite etc) don't have to load all of `preline.js` - Fixed an error about invalid nullish statement Public methods selectValue() and selectAttribute - selectedItem() returns elemnent that was selected - selectedValue() returns the value. - selectedAttr() returns the custom attribute. One option to the ComboBox options: - apiSearchQueryTransformer to allow transforming query before fetch package.json: Few watch commands and generation scripts added.

mohsinhijazee · 2024-10-24T17:05:23Z

@olegpix Thank you for your feedback!

I have removed everything that was not relevant (package.json scripts, generated dist/ and other d.ts files including the change related to select plugin) so I hope it is easier to review now.

Also, about packaging and bundling, I think some scripts or steps should be documented within the package.json etc. Not so sure about it. What steps/process do you guys follow for that?

olegpix · 2024-11-04T15:05:11Z

@olegpix Thank you for your feedback!

I have removed everything that was not relevant (package.json scripts, generated dist/ and other d.ts files including the change related to select plugin) so I hope it is easier to review now.

Also, about packaging and bundling, I think some scripts or steps should be documented within the package.json etc. Not so sure about it. What steps/process do you guys follow for that?

Hi,
The process is quite simple, you need to run 3 tasks from the Preline root (consistently):

npm run build
npm run build:mjs (optional)
npm run generate-dts
That's it.

We'll check and approve your PR after the next update, now we are focused on some other tasks.
Thank you for your work!

jahaganiev · 2024-12-04T00:11:06Z

Hey @mohsinhijazee - thanks a lot for the PR, we will try to include this PR in v2.7.0 update. Thanks!

lenart · 2024-12-21T17:58:25Z

src/plugins/combobox/index.ts

 	private init() {
+		// So that not to dpeendon preloading whole library.


Small typo in the comment.

olegpix · 2025-02-18T12:53:52Z

src/plugins/combobox/index.ts

+		if (!query) return null;
+
+		if (typeof query === 'string') {
+			return eval(query) as QueryTransformer;


Hi @mohsinhijazee , thanks for corrections.

I noticed that you've added eval in the parseApiQueryTransformer method. eval is known to be a potential security risk, especially when handling dynamic inputs, as it can allow arbitrary code execution.

This raises some concerns on our side, as we strive to keep the codebase secure and avoid introducing vulnerabilities. Could you please explain why eval was necessary here and if there might be a safer alternative?

We need to ensure that all code changes are secure and well-justified, so your clarification would be appreciated.

Thanks.

Yes, this eval is used so that the users of the component can utilise data-* attributes to setup the search function (transforming the query before it is sent to the server) without going to write further Javascript (and instantiating a control in pure JS) therefore this does not come from the user input per se, rather from the developer/user of the library and I believe this is a safe use case and can't be exploited because it is not linked to an input control that can be manipulated by the end users to change things around.

Even data attribute though dynamically can be changed, but that change won't trigger the eval because combobox would be initialised already.

Other alternative is to drop the ability of being able to supply a search function via data-* attributes which would result in a component that is less capable when used in typical HTML only scenarios.

mohsinhijazee force-pushed the combobox-enhancements branch 3 times, most recently from e6ea55b to 1664ee2 Compare October 10, 2024 17:25

olegpix requested changes Oct 23, 2024

View reviewed changes

mohsinhijazee force-pushed the combobox-enhancements branch from 1664ee2 to cfe7ee6 Compare October 24, 2024 16:58

mohsinhijazee added 3 commits October 24, 2024 19:01

Reverting irrelevant changes.

d6c9487

Using built in isArray.

c742d2c

mohsinhijazee force-pushed the combobox-enhancements branch from cfe7ee6 to c742d2c Compare October 24, 2024 17:01

lenart reviewed Dec 27, 2024

View reviewed changes

src/plugins/combobox/index.ts

private init() {

// So that not to dpeendon preloading whole library.

Copy link

lenart Dec 21, 2024

Choose a reason for hiding this comment

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

Small typo in the comment.

olegpix reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ComboBox - New methods + bullet proofing #479

ComboBox - New methods + bullet proofing #479

mohsinhijazee commented Oct 10, 2024 •

edited

Loading

olegpix left a comment

olegpix Oct 23, 2024

mohsinhijazee Oct 24, 2024

olegpix Oct 23, 2024

mohsinhijazee Oct 24, 2024

olegpix Oct 23, 2024

olegpix Oct 23, 2024

olegpix Oct 23, 2024

olegpix Oct 23, 2024

olegpix Oct 23, 2024

olegpix Oct 23, 2024

mohsinhijazee Oct 24, 2024

olegpix Oct 23, 2024

mohsinhijazee Oct 24, 2024

olegpix Oct 23, 2024

mohsinhijazee commented Oct 24, 2024

olegpix commented Nov 4, 2024

jahaganiev commented Dec 4, 2024

lenart Dec 21, 2024

olegpix Feb 18, 2025

mohsinhijazee Feb 21, 2025

		private init() {
		// So that not to dpeendon preloading whole library.

ComboBox - New methods + bullet proofing #479

Are you sure you want to change the base?

ComboBox - New methods + bullet proofing #479

Conversation

mohsinhijazee commented Oct 10, 2024 • edited Loading

olegpix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohsinhijazee commented Oct 24, 2024

olegpix commented Nov 4, 2024

jahaganiev commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohsinhijazee commented Oct 10, 2024 •

edited

Loading