josdejong / svelte-jsoneditor

A web-based tool to view, edit, format, repair, query, transform, and validate JSON
https://jsoneditoronline.org
Other
897 stars 108 forks source link

Feature request: onValidated() #170

Closed RyKilleen closed 1 year ago

RyKilleen commented 1 year ago

Hey there, as always thanks for the great work! This is a tangental follow-up to #128 which has been serving us very well!

The challenge we're facing:

In the Editor life-cycle, we're handling some external state based on onChange. We have a validator that fires when changes are made, and life is good.

When a user uses text-mode, our onChange fires regardless of validation and we receive errors because of incomplete JSON returned. I'm seeking the best way to handle firing external events only on valid changes, without having to duplicate validation logic that's provided to the editor. Were trying to make the partially-edited JSON text experience a little bit smoother.

In short, a lifecycle method that only fires on valid changes would be a big help!

RyKilleen commented 1 year ago

There are a few spots in the editor component where validateContentType is called with an if statement, perhaps consolidating those and calling onValidChange in an else is a useful approach?

I've plenty of React experience but no Svelte experience, still happy to try and PR!

josdejong commented 1 year ago

Thanks, good to hear :)

The onChange event also passes contentErrors, which can contain parse errors and also validation errors when you've defined a validator. Is that what you're looking for?

josdejong commented 1 year ago

ow I think we've had this discussion before 😂 , see https://github.com/josdejong/svelte-jsoneditor/issues/128#issuecomment-1220956679

RyKilleen commented 1 year ago

It was definitely a side chat in that larger discussion haha!

Our validation function is significant and is working on significant-sized JSON. Calling it manually in onChange when it's already been called isn't ideal. We've managed to side-step this issue a little by checking that the value for textmode onChange at least parses as JSON before firing, but feels a bit redundant.

This feels like a nice convenience feature to expose and I'm happy to tackle it if you think it's worthwhile!

josdejong commented 1 year ago

In short, a lifecycle method that only fires on valid changes would be a big help!

We've managed to side-step this issue a little by checking that the value for textmode onChange at least parses as JSON before firing, but feels a bit redundant.

I think you can just check whether contentErrors in the onChange callback contains a parseError or not:

function onChange(updatedContent, previousContent, { contentErrors }) {
  if (!isContentParseError(contentErrors)) {
    // we have valid JSON -> run validation
  } else {
    // we have invalid JSON -> do not run validation
  }
}

Our validation function is significant and is working on significant-sized JSON.

Ah, now I understand the context better. So I think you are now running validation externally of the editor on your own conditions instead of on every change? If that is the case, we should probably give more control over when svelte-jsoneditor runs validation if the API is not sufficient. Thinking aloud though, I think you can already do that. In the validator function, you can decide to to run validation (expensive) or return the previous, cached results and set a flag "dirty" to true (cheap). On your own conditions, for example when the user clicks a "Save" button, you can verify whether the validation results are marked "dirty", and if so, call .validate() first before continuing. Can you explain how your flow currently is with the heavy validate function?

RyKilleen commented 1 year ago

Ah I think it's a typing issue, I didn't realize that ContentErrors included both parsing and validating errors! Didn't think to check either.

I'll give that a shot and that very likely solves what I'm trying to do here, thanks!

RyKilleen commented 1 year ago

That does the trick, thanks so much.

josdejong commented 1 year ago

😎