feat: implement file registry on filepicker #1246

vsgoulart · 2024-08-25T18:31:31Z

Related to #1239

This PR adds a new form-js element or visually changes an existing component.
- => In that case, we need to ensure we follow up on this, e.g. by creating an issue in Tasklist

Skaiir · 2024-08-26T04:47:11Z

packages/form-js-viewer/src/Form.js

@@ -168,9 +168,12 @@ export class Form {

    const errors = this.validate();

+    const files = this.get('fileRegistry').getAllFiles();


I think this needs to be filtered based on the available instances. Otherwise it'll submit hidden files as well.

Lazy pseudocode:

const validFileRefs = instances .filter(instance => instance.type === 'filepicker') .map(instance => instance.value); const filteredFiles = files.filter(file => validFileRefs.includes(file.value));

But first there's the issue of how these files are keyed (see other comment).

I deleted the files when the component unmounts, this the logic is encapsulated on the component

Skaiir · 2024-08-26T04:53:41Z

packages/form-js-viewer/src/render/components/form-fields/FilePicker.js

  const { field, onChange, domId, errors = [], disabled, readonly, required } = props;
  const { label, multiple = '', accept = '', id } = field;
  const evaluatedAccept = useSingleLineTemplateEvaluation(accept);
  const evaluatedMultiple =
    useSingleLineTemplateEvaluation(typeof multiple === 'string' ? multiple : multiple.toString()) === 'true';
  const errorMessageId = `${domId}-error-message`;
+  const filesKey = `${id}_value_key`;


Keying needs to be done differently. It needs to include the full path otherwise dynamic list keys will overwrite eachother. The id is unique only at the design level. The true id of an element at the render level is the full path / key of the element.

My prefered fileKey would be something like fileRef::${simpleHash(fullPath)}. Hashing is optional but it's more about the fact that it's mental overhead for the users for it to be actually readable. Also I know some people will try to reverse engineer the path from it which is not something I'd like to encourage.

I didn't do the hashing, I'm not sure if I agree with the mental overhead point.
Anyway, this can be easily fixed tomorrow.

I guess on this one we can defer to @volodymyr-melnykc for a decision.

Skaiir · 2024-08-26T04:58:25Z

packages/form-js-viewer/src/render/components/form-fields/FilePicker.js

          setSelectedFiles(files);
        }}
      />
      <div className="fjs-filepicker-container">
        <button
          type="button"
-          disabled={disabled}
+          disabled={disabled || readonly || fileRegistry === null}


Just FYI, at the moment it's still possible to submit a file in the editor. I guess you're planning on adding a custom editor renderer for this anyways?

If not, needs fixing for sure. I don't know if it's related to the label or something but when I click around on it in the editor, it opens the window, which can lead to an exception.

As I mentioned through Slack I couldn't reproduce it, maybe we can check it together tomorrow

Skaiir · 2024-08-27T14:51:02Z

packages/form-js-playground/test/spec/form.json

@@ -2,256 +2,37 @@
  "$schema": "../../../form-json-schema/resources/schema.json",
  "components": [
    {
-      "type": "expression",


This is the main start:playground schema, probably committed by error?

Skaiir · 2024-08-27T14:55:21Z

packages/form-js-viewer/src/render/components/form-fields/FilePicker.js


  useEffect(() => {
-    const reset = () => {
+    return () => {


The problem with this is that by design, hiding operations are not destructive, so that if someone hides something by accident they don't lose their progress. So I think we'll have to go the route I initially suggested as otherwise we orphan file references and have different behavior for the file picker and regular components.

This is what happened when I hide and show something again.

Skaiir · 2024-08-27T14:56:18Z

packages/form-js-viewer/src/render/components/form-fields/FilePicker.js

  const { field, onChange, domId, errors = [], disabled, readonly, required } = props;
  const { label, multiple = '', accept = '', id } = field;
  const evaluatedAccept = useSingleLineTemplateEvaluation(accept);
  const evaluatedMultiple =
    useSingleLineTemplateEvaluation(typeof multiple === 'string' ? multiple : multiple.toString()) === 'true';
  const errorMessageId = `${domId}-error-message`;
+  const filesKey = `${id}_value_key`;


I guess on this one we can defer to @volodymyr-melnykc for a decision.

Skaiir · 2024-09-04T13:27:10Z

packages/form-js-viewer/src/features/repeatRender/index.js


 export const RepeatRenderModule = {
  __init__: ['repeatRenderManager'],
  repeatRenderManager: ['type', RepeatRenderManager],
+  fileRegistry: ['type', FileRegistry],


Just for future reference, we're registering this twice which I guess was due to a timing issue, but the modules access eachother. So probably here you needed either a __depends__ or to re-order some things.

Related to #1239

…n hide if events

Related to #1239

* feat: implement file registry on filepicker * fix: fix file registry key * fix: delete file when dynamic list item is deleted * test: fix filepicker tests * chore: revert unnecessary change * fix: properly handle dynamic lists deletion * fix: set input file value after expanding dynamic list items * test: fix tests * fix: trim all files from a dynamic list subtree * fix: refactor form field instance registry * chore: added `useBooleanExpressionEvaluation` hook * fix: improve file picker reliability Related to #1239 * chore: changed filepicker prefix to `files::` Related to #1239 * fix: ensure the hidden filepicker gets disabled as well Related to #1239 * chore: adjust filepicker tests * chore: cleanup formFieldInstanceRegistry tests Related to #1239 * refactor: Improve typing * refactor: Improve variable naming * fix: fix dynamic list expand button label * fix: prevent collapsed dynamic list items from unmounting * refactor: Remove unnecessary method, improve types and delete files on hide if events * test: remove duplicated test * fix: clear file references to deleted files Related to #1239 --------- Co-authored-by: Skaiir <[email protected]>

vsgoulart requested a review from Skaiir August 25, 2024 18:31

bpmn-io-tasks bot added the needs review Review pending label Aug 25, 2024

bpmn-io-tasks bot assigned vsgoulart Aug 25, 2024

Skaiir requested changes Aug 26, 2024

View reviewed changes

bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 26, 2024

vsgoulart requested a review from Skaiir August 26, 2024 18:36

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Aug 26, 2024

Skaiir requested changes Aug 27, 2024

View reviewed changes

bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 27, 2024

vsgoulart force-pushed the 1239-file-input-element branch from 68dea73 to 4397dd6 Compare September 2, 2024 12:40

vsgoulart added 4 commits September 2, 2024 15:48

feat: implement file registry on filepicker

29b2ab4

fix: fix file registry key

9adcef4

fix: delete file when dynamic list item is deleted

d8856b6

test: fix filepicker tests

d78ecc5

vsgoulart force-pushed the 1239-file-input-element-file-registry branch from 6ae8c2a to d78ecc5 Compare September 2, 2024 12:48

vsgoulart added 5 commits September 2, 2024 15:54

chore: revert unnecessary change

47df9ab

fix: properly handle dynamic lists deletion

4ce9952

fix: set input file value after expanding dynamic list items

ae551a7

test: fix tests

e815aa2

fix: trim all files from a dynamic list subtree

093c725

vsgoulart requested a review from Skaiir September 3, 2024 18:41

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Sep 3, 2024

Skaiir reviewed Sep 4, 2024

View reviewed changes

Skaiir added 2 commits September 5, 2024 10:18

fix: refactor form field instance registry

ff19c18

chore: added useBooleanExpressionEvaluation hook

596aff5

Skaiir force-pushed the 1239-file-input-element-file-registry branch from d0f0455 to 33d404c Compare September 5, 2024 09:20

Skaiir added 2 commits September 5, 2024 11:22

fix: improve file picker reliability

c4bdaac

Related to #1239

chore: changed filepicker prefix to files::

df63741

Related to #1239

Skaiir force-pushed the 1239-file-input-element-file-registry branch from 33d404c to df63741 Compare September 5, 2024 09:23

Skaiir and others added 9 commits September 5, 2024 11:28

fix: ensure the hidden filepicker gets disabled as well

8b36418

Related to #1239

chore: adjust filepicker tests

66fcf4f

chore: cleanup formFieldInstanceRegistry tests

53c0b51

Related to #1239

refactor: Improve typing

59549dc

refactor: Improve variable naming

62dd997

fix: fix dynamic list expand button label

5899ff6

fix: prevent collapsed dynamic list items from unmounting

f8c8137

refactor: Remove unnecessary method, improve types and delete files o…

99efb59

…n hide if events

test: remove duplicated test

17a535f

Skaiir self-requested a review September 6, 2024 15:22

Skaiir approved these changes Sep 6, 2024

View reviewed changes

fix: clear file references to deleted files

0bf66bf

Related to #1239

vsgoulart force-pushed the 1239-file-input-element-file-registry branch from 0d63501 to 0bf66bf Compare September 9, 2024 08:41

vsgoulart merged commit e4ff280 into 1239-file-input-element Sep 9, 2024
11 of 12 checks passed

bpmn-io-tasks bot removed the needs review Review pending label Sep 9, 2024

vsgoulart deleted the 1239-file-input-element-file-registry branch September 9, 2024 08:48

Skaiir mentioned this pull request Sep 10, 2024

[Implement] Create filepicker input component #1239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement file registry on filepicker #1246

feat: implement file registry on filepicker #1246

vsgoulart commented Aug 25, 2024

Skaiir Aug 26, 2024 •

edited

Loading

vsgoulart Aug 26, 2024

Skaiir Aug 26, 2024

vsgoulart Aug 26, 2024

Skaiir Aug 27, 2024

Skaiir Aug 26, 2024

Skaiir Aug 26, 2024

vsgoulart Aug 26, 2024

Skaiir Aug 27, 2024

vsgoulart Aug 27, 2024

Skaiir Aug 27, 2024 •

edited

Loading

Skaiir Aug 27, 2024

Skaiir Sep 4, 2024

		@@ -168,9 +168,12 @@ export class Form {

		const errors = this.validate();

		const files = this.get('fileRegistry').getAllFiles();

feat: implement file registry on filepicker #1246

feat: implement file registry on filepicker #1246

Conversation

vsgoulart commented Aug 25, 2024

Skaiir Aug 26, 2024 • edited Loading

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

Skaiir Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skaiir Aug 26, 2024 •

edited

Loading

Skaiir Aug 27, 2024 •

edited

Loading