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: propagate invalid values to setValue #252

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Conversation

marstamm
Copy link
Contributor

@marstamm marstamm commented Jun 14, 2023

Whether invalid values are persisted on errors or not should be a concern of the application. This PR introduces that we also propagate invalid values to the app.

Integrates into bpmn-js-properties-panel: bpmn-io/bpmn-js-properties-panel#938

related to bpmn-io/bpmn-js-properties-panel#928
related to camunda/camunda-modeler#3357

@marstamm marstamm requested a review from a team June 14, 2023 12:00
@marstamm marstamm self-assigned this Jun 14, 2023
@marstamm marstamm requested review from philippfromme and smbea and removed request for a team June 14, 2023 12:00
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Jun 14, 2023
@marstamm marstamm force-pushed the propagate-invalid-values branch from c83cc57 to c766f0d Compare June 14, 2023 12:02
Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

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

So we now validate twice? We could add an optional error parameter to setValue instead.

@marstamm
Copy link
Contributor Author

marstamm commented Jul 4, 2023

So we now validate twice?

Not necessarily. In most cases, the validation is only done as linting and not performed on setValue. Only for IDs we will perform the check twice.

I like the Idea to also provide the validation error in the setValue callback so we don't need to validate twice. I'll add it

@marstamm marstamm force-pushed the propagate-invalid-values branch from c766f0d to 61a1dbb Compare July 4, 2023 08:09
@marstamm
Copy link
Contributor Author

marstamm commented Jul 4, 2023

Added the suggested improvement via 61a1dbb

@marstamm marstamm requested a review from philippfromme July 4, 2023 08:10
Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

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


// given
const setValueSpy = sinon.spy();
const validate = (v) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we use this pattern a lot, but these validation functions are pointless. We know exactly if the validation is to return an error or not. So why not just do const validate = () => 'error'? I'll file a follow-up pull request simplifying this. Our tests should stay focused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might have been deliberate in the beginning (e.g. only set "invalid" when the check actually fails. However, we never do the // assume - valid at the start part, so we should be good to simplify

@marstamm marstamm merged commit 425c1f6 into main Jul 5, 2023
@marstamm marstamm deleted the propagate-invalid-values branch July 5, 2023 09:17
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 5, 2023
smbea added a commit to bpmn-io/bpmn-js-element-templates that referenced this pull request Aug 3, 2023
smbea added a commit to bpmn-io/bpmn-js-element-templates that referenced this pull request Aug 3, 2023
fake-join bot pushed a commit to bpmn-io/bpmn-js-element-templates that referenced this pull request Aug 7, 2023
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