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

Create a helper that normalizes a value to an array (if not already an array) #7401

Closed
oleq opened this issue Jun 8, 2020 · 5 comments · Fixed by #8449
Closed

Create a helper that normalizes a value to an array (if not already an array) #7401

oleq opened this issue Jun 8, 2020 · 5 comments · Fixed by #8449
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:utils type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@oleq
Copy link
Member

oleq commented Jun 8, 2020

Provide a description of the task

We use the following normalization Array.isArray( foo ) ? foo : [ foo ]; many times across the code-base. I think we need an utils for this repeating pattern.

packages/ckeditor5-engine/src/conversion/conversion.js:
   77: 		this._downcast = Array.isArray( downcastDispatchers ) ? downcastDispatchers : [ downcastDispatchers ];
   80: 		this._upcast = Array.isArray( upcastDispatchers ) ? upcastDispatchers : [ upcastDispatchers ];
  640: 		upcastAlso = Array.isArray( upcastAlso ) ? upcastAlso : [ upcastAlso ];

packages/ckeditor5-engine/src/conversion/downcasthelpers.js:
  850: 				const classes = Array.isArray( oldAttribute.value ) ? oldAttribute.value : [ oldAttribute.value ];
  869: 				const classes = Array.isArray( newAttribute.value ) ? newAttribute.value : [ newAttribute.value ];

packages/ckeditor5-engine/src/view/element.js:
  730: 		className = Array.isArray( className ) ? className : [ className ];
  748: 		className = Array.isArray( className ) ? className : [ className ];
  795: 		property = Array.isArray( property ) ? property : [ property ];

packages/ckeditor5-image/src/image/imageinsertcommand.js:
  54: 			const sources = Array.isArray( options.source ) ? options.source : [ options.source ];

packages/ckeditor5-image/src/imageupload/imageuploadcommand.js:
  64: 			const filesToUpload = Array.isArray( options.file ) ? options.file : [ options.file ];

packages/ckeditor5-watchdog/src/contextwatchdog.js:
  237: 		const itemConfigurations = Array.isArray( itemConfigurationOrItemConfigurations ) ?
  313: 		const itemIds = Array.isArray( itemIdOrItemIds ) ?

packages/ckeditor5-widget/src/utils.js:
  120: 		return Array.isArray( classes ) ? classes : [ classes ];
@oleq oleq added type:task This issue reports a chore (non-production change) and other types of "todos". intro Good first ticket. package:utils squad:red labels Jun 8, 2020
@niegowski
Copy link
Contributor

I was wondering why we are not using [].concat( classes ) ? Result is the same as for Array.isArray( classes ) ? classes : [ classes ]

@oleq
Copy link
Member Author

oleq commented Jun 9, 2020

Not a very versatile test but this is probably the reason why

@niegowski
Copy link
Contributor

niegowski commented Jun 9, 2020

Not a very versatile test but this is probably the reason why

And for classes list... do we ever expect it to be more than 2 or 3? 😉

Ok, it's not the case of how many classes but how many calls

@oleq
Copy link
Member Author

oleq commented Jun 9, 2020

AFAIR @jodator researched this in the past so maybe he would like to chime in.

@jodator
Copy link
Contributor

jodator commented Jun 9, 2020

From the performance perspective it might be a case that Array.concat() might do many things under the hood (checking for params types, if it is array then do other things, etc) and the simple call is faster (but longer in code) for this case.

@mlewand mlewand added the domain:dx This issue reports a developer experience problem or possible improvement. label Jun 29, 2020
@mlewand mlewand added this to the backlog milestone Jun 29, 2020
@Reinmar Reinmar modified the milestones: backlog, iteration 38 Oct 26, 2020
@psmyrek psmyrek self-assigned this Nov 9, 2020
jodator added a commit that referenced this issue Nov 20, 2020
Internal: Added `toArray()` helper to cast any value to an array. Closes #7401.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:utils type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants