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.

Sign up for GitHub

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

Jump to bottom

Implement JS Function / Scripting Component #1100

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Draft

Implement JS Function / Scripting Component #1100

wants to merge 11 commits into from

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Mar 19, 2024 •
edited
Loading

Closes #1102

@Skaiir Skaiir self-assigned this Mar 19, 2024
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Mar 19, 2024
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 19, 2024 06:27 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 19, 2024 06:46 Destroyed
Base automatically changed from 1037-hidden-fields-fix-playground-root to 1073-hidden-fields-schema March 20, 2024 09:42
Base automatically changed from 1073-hidden-fields-schema to 1073-hidden-fields-tests March 20, 2024 09:42
Base automatically changed from 1073-hidden-fields-tests to 1073-hidden-fields-impl March 20, 2024 09:43
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 20, 2024 11:34 Destroyed
Base automatically changed from 1073-hidden-fields-impl to develop March 20, 2024 11:36
@Skaiir Skaiir changed the title WIP WIP: script component Mar 20, 2024
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 20, 2024 12:17 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 22, 2024 07:02 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 25, 2024 04:46 Destroyed
@Skaiir Skaiir changed the title WIP: script component Implement script component Mar 25, 2024
@Skaiir Skaiir changed the title Implement script component Implement script component (WIP) Mar 25, 2024
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 25, 2024 05:00 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 25, 2024 05:37 Destroyed
@volodymyr-melnykc
Copy link

volodymyr-melnykc commented Mar 25, 2024 •
edited
Loading

@Skaiir The toggle "Do not submit" can be unclear, especially when placed before the input for function.
I suggest moving it down to be after the field for Javascript code and updating the text to:

  • Do not submit the function's result with the form submission

Another suggestion is to update the explanation text for function parameters:

  • From: "Define the parameters to pass to the javascript sandbox."
  • To: "Define the parameters to pass to the javascript function."

@volodymyr-melnykc
Copy link

@Skaiir Not directly related to the JS component but good to do.

Can we update the icons for the iFrame and Group components to the new ones in Figma? As well as reorder them: (1) Group, (2) Dynamic list, (3) iFrame?
And I can update the documentation (to reflect visual updates).

image

Let me know if it's better to handle as a separate issue.

@github-actions github-actions bot temporarily deployed to demo-js-script-component March 25, 2024 17:27 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component March 25, 2024 17:32 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component April 2, 2024 06:04 Destroyed
vsgoulart
vsgoulart reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

@Skaiir In the future it would be better to split into a PR for properties panel, one for the schema package and for the editor/viewer

packages/form-js-viewer/src/render/components/icons/JSFunction.svg Outdated Show resolved Hide resolved
packages/form-js-editor/src/render/components/editor-form-fields/EditorJSFunctionField.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/JSFunctionField.js Outdated

const [ sandbox, setSandbox ] = useState(null);
const [ hasRunLoad, setHasRunLoad ] = useState(false);
const [ iframeContainerId ] = useState(`fjs-sandbox-iframe-container_${uuidv4()}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Douglas, it looks like we don't really need this.
If it's really necessary we could try to replace it with crypto.randomUUID()

packages/form-js-viewer/test/spec/render/components/form-fields/JSFunctionField.spec.js Outdated
});

// wait for the iframe to compute the expression and pass it back
await new Promise(r => setTimeout(r, 100)).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use waitFor from testing library here I think

packages/form-js-viewer/src/render/components/form-fields/JSFunctionField.js Outdated Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/JSFunctionField.js Show resolved Hide resolved
packages/form-js-viewer/src/render/components/form-fields/JSFunctionField.js
Websandbox.connection.setLocalApi({ compute: ___executeUserCode___ });
`;

const _sandbox = Sandbox.create(hostAPI, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you update the sandbox instead of always creating a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually want to completely destroy it, to make sure all user code event subscriptions get wiped as they are working on it in the playground. In the viewer this will only run once.

@vsgoulart
Copy link
Contributor

@Skaiir We need to rebase this branch

@vsgoulart
Copy link
Contributor

@volodymyr-melnykc do we track the use of the form components anywhere? I think we can do some really interesting things, but I don't see most users using it because it looks a bit too complex to me.

@volodymyr-melnykc
Copy link

volodymyr-melnykc commented Apr 5, 2024 •
edited
Loading

@volodymyr-melnykc do we track the use of the form components anywhere? I think we can do some really interesting things, but I don't see most users using it because it looks a bit too complex to me.

@vsgoulart We implemented the event tracking here: https://github.com/camunda/team-hto/issues/350

I prepared the report about form components usage. It's available here:
https://eu.mixpanel.com/s/3256O2

@github-actions github-actions bot temporarily deployed to demo-js-script-component April 8, 2024 07:14 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component April 8, 2024 07:19 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component April 8, 2024 07:30 Destroyed
@github-actions github-actions bot temporarily deployed to demo-js-script-component April 8, 2024 11:36 Destroyed
vsgoulart
vsgoulart reviewed Apr 10, 2024
View reviewed changes
Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

@Skaiir Just one small comment left

We have a bug on the table component, we need to validated the values to see if they're strings because this crashes the entire app
image

packages/form-js-viewer/test/spec/render/components/form-fields/JSFunctionField.spec.js
});

// wait for the iframe to compute the expression and pass it back
await new Promise(r => setTimeout(r, 100)).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also replace the other occurrences with the waitFor I mentioned in the other comments

@Skaiir Skaiir marked this pull request as draft May 2, 2024 06:32
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels May 2, 2024
@Skaiir
Copy link
Contributor Author

Skaiir commented May 7, 2024

Deprioritized due to the re-architecture project.

@Skaiir Skaiir added backlog Queued in backlog and removed in progress Currently worked on labels May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@douglasbouttell-camunda douglasbouttell-camunda douglasbouttell-camunda left review comments

@vsgoulart vsgoulart vsgoulart left review comments

Assignees

@Skaiir Skaiir

Labels
backlog Queued in backlog deploy-preview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement script component
6 participants
@Skaiir @volodymyr-melnykc @christian-konrad @marcosgvieira @vsgoulart @douglasbouttell-camunda