search

surveyjs / survey-library

Free JavaScript form builder library with integration for React, Angular, Vue, jQuery, and Knockout.
https://surveyjs.io/form-library
MIT License
4.02k stars 785 forks source link

Property changed handlers #8217

Closed SamMousa closed 2 months ago

SamMousa commented 2 months ago

Are you requesting a feature, reporting a bug or asking a question?

Question

What is the current behavior?

There's a lot of functions related to watching changes of properties:

It's not clear to me what the differences are and which ones I should use.

JaneSjs commented 2 months ago

Hi @SamMousa, Thank you for your inquiry. I would be in a better position to address your inquiry if you could elaborate on your usage scenario. Do you want to track property change in a creator?

I look forward to your reply.

SamMousa commented 2 months ago

Both during survey creation and runtime. (It's fine if there are different approaches for both)

JaneSjs commented 2 months ago

Thank you for the update.

Regarding the runtime:

Consider the following code:

const popupSurveyModel = new Survey.PopupSurveyModel(null, survey);

popupSurveyModel.registerPropertyChangedHandlers(["isShowing"], 
(newValue) => {
  if(newValue === false)
    alert("Popup is closed!");
});

Also, you may use the registerPropertyChangedHandlers function to attach a function to a question object. Consider the following code:

options.question.registerPropertyChangedHandlers(["title"], function (newValue) {
      //...
});

Please let me know if you have further questions.

SamMousa commented 2 months ago

Yes, can you update the documentation?

The registerFunctionOnPropertyValueChanged function is outdated and should not be used.

It should be marked as @deprecated in the source code.

To handle changes of a survey and its elements' properties at design time, use the creator.onSurveyPropertyValueChanged event.

If I want both, is there any harm in always using the survey.onPropertyChanged?

You may wish to use the registerPropertyChangedHandlers function to implement a function which would fire on a property change of a Base SurveyJS object instance: a survey, question.

This seems like a duplicate functionality with respect to onPropertyChanged, or am I misunderstanding? It'd be great if there was some documentation pointing developers to the preferred implementation method. When I look at the code onPropertyChanged uses a proper event implementation in a separate class, while registerPropertyChangedHandlers seems to be an implementation that lives inside the base class.

I know I can use both; but please document somewhere (not just in an answer on this ticket) which considerations there are and which I should use.

JaneSjs commented 2 months ago

Hi Sam, Thank you for the update.

It should be marked as @deprecated in the source code.

@RomanTsukanov, please take a look.

If I want both, is there any harm in always using the survey.onPropertyChanged?

The survey.onPropertyChanged is raised when a survey object property is changed. However, this event won't be raised when a question property is changed. If you wish to handle the property change or survey and its elements in SurveyJS Creator, use the creator.onSurveyPropertyValueChanged event.

RomanTsukanov commented 2 months ago

Hi Sam,

It should be marked as @deprecated in the source code.

Our source code is not an official source of information about available APIs. Please use documentation on our website.

SamMousa commented 2 months ago

I'm sorry but this kind of comment really makes no sense. You generate types in d.ts files. Why generate them if they are not supposed to be used. The more I try to interact with the code and try to help and improve it the more I get the feeling that SurveyJS is open source in name only...

If you look at the contributor graphs (https://github.com/surveyjs/survey-library/graphs/contributors) it's only your employees putting in the work and there's no attempt to grow the community part. All suggestions I do objectively make the code better and in most cases I'm willing to do the legwork. But there's just so much resistance to outside contributions that it seems impossible to get thing merged.

Even if the source code is not an official source, it is still a source and it can be improved by marking something that you don't want code consumers to use with @internal...

SamMousa commented 2 months ago

Also, the documentation is inconsistent as well, for example this: https://surveyjs.io/survey-creator/documentation/api-reference/survey-creator#onShowingProperty https://surveyjs.io/survey-creator/documentation/api-reference/survey-creator#onCanShowProperty

2 different properties, but internally they are the same:

public onShowingProperty: EventBase<SurveyCreatorModel, PropertyAddingEvent> = this.addCreatorEvent<SurveyCreatorModel, PropertyAddingEvent>();
public onCanShowProperty: EventBase<SurveyCreatorModel, PropertyAddingEvent> = this.onShowingProperty;
RomanTsukanov commented 2 months ago

I completely agree that SurveyJS TypeScript definitions could be improved. However, this should be a well-planned and meticulous work—marking just one member as deprecated won't do. This work should take into account many factors, including necessary changes to our documentation-generating scripts. At the current moment, this isn't in our roadmap.

2 different properties, but internally they are the same:

onCanShowProperty is an obsolete name for the onShowingProperty event. It got into the docs by mistake. Thanks for pointing this out, we'll remove it.

Regarding your first question on this thread: I've updated the onPropertyChanged and registerPropertyChangedHandlers descriptions to clarify the difference between these API members in this PR: https://github.com/surveyjs/survey-library/pull/8241. These changes will appear on our website with the next update. Since the main question has been addressed, I'm gonna close this issue. If you have any questions left, fell free to reopen it or create a new one.

Thanks!