Reduce side effects & global variable usage

[SamMousa]

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

Question, about the code quality and the openness to external contributions

What is the current behavior?

A lot of files define a class and also instantiate a singleton instance of that class. For example (this comes from the creator): editorLocalization.ts

export class EditorLocalization {
...
}
export var editorLocalization = new EditorLocalization();

Another example: surveyStrings.ts

export var surveyLocalization = {
...
}
export var surveyStrings = englishStrings;
(<any>surveyLocalization).locales["en"] = englishStrings;
(<any>surveyLocalization).localeNames["en"] = "english";

Not only are these side effects, they also use global variables.

What is the expected behavior?

When importing code I expect to get definitions, because of the way this code is written I not only get definitions, but also get side effects.

This has a number of downsides that make it complicated to use SurveyJS in a clean manner, and it also limits some use cases that are not the standard use case:

• Importing specific exports from these files may or may not run the side effects. For example it is possible to get a references to the localization object before the localizations themselves have been loaded
• It is impossible to have multiple creators / surveys with different translations because they use the same global variables
• It is harder to reason about application state since any code may or may not alter these global variables

I'd love to (be able) to contribute more, but because of these global variables it is hard to reason about the application state. It means that any code may directly access some data structure and alter it; this means that during a refactor we have to always make sure that all kinds of access to this global variable still work.

Reducing the number of global variables would significantly simplify reasoning and improve code cleanliness. It can be done incrementally and slowly, there is no rush.

Would you be open to PRs with this goal in mind? I've already created one (https://github.com/surveyjs/survey-library/pull/7325) that improves access to the translation class. It doesn't resolve all the issue above, but it's a step.

[tsv2013]

First of all I want to tell you that we highly appreciate your contribution and yor valuable feedback. Our team shares your ideas concering the architecture improvement of the products. And we arecontinuously working on the it.

Meanwhile we'd like to learn more abou the problems you are faced then using our products to understand what exact problems will solve this or that pull request. In this case we will have more understanding what are you trying to achieve and find a best way for it. And of course pull requests require time to understand what is going on, whether they contain breaking changes, API changes and so on.

It would be nice to have a general flow for contributions:

• initial issue on GitHub describing the problem
• discussion how it should be solved in the best way
• desicion - who (we or you) will make a pull request to resolve it
• PR shouls has unit or other tests for the implemented functionality
• PR should pass al checks and code review and then be merged

@andrewtelnov What do you think?

[tsv2013]

@SamMousa I've added some comments to the https://github.com/surveyjs/survey-library/pull/7325

[SamMousa]

@SamMousa I've added some comments to the #7325

I think you still need to submit them ;-)

[SamMousa]

First of all I want to tell you that we highly appreciate your contribution and yor valuable feedback. Our team shares your ideas concering the architecture improvement of the products. And we arecontinuously working on the it.

Thanks! I'm very grateful for the great product you've created, don't ever let my comments give you any other impression.

Meanwhile we'd like to learn more abou the problems you are faced then using our products to understand what exact problems will solve this or that pull request. In this case we will have more understanding what are you trying to achieve and find a best way for it.

Currently I'm working on wrapping the creator (knockout) and runner (knockout) in a Svelte app. We want to customize a lot (any relevant changes I try to push back upstream). Most issues I run into I can work around, it's just a lot of extra effort to work around things and I'd prefer to spend that time contributing the library instead. (And in return this makes it easier for me and everyone else to integrate)

And of course pull requests require time to understand what is going on, whether they contain breaking changes, API changes and so on.

It would be nice to have a general flow for contributions:

• initial issue on GitHub describing the problem
• discussion how it should be solved in the best way
• desicion - who (we or you) will make a pull request to resolve it
• PR shouls has unit or other tests for the implemented functionality
• PR should pass al checks and code review and then be merged

I 100% agree with this on simple issues, but this flow doesn't work for more abstract things like architecture issues. First of all they probably can't and shouldn't be fixed in a single PR. With this issue I'm looking more for a general standard for the library on how we wish things to be.

For example, if we decide that we no longer want to have global objects, it doesn't mean the next PR has to refactor everything and remove all of them. Instead we could add some new rules that allow us to reach the goal eventually:

1. Don't add new globals
2. Don't allow new access to globals
3. etc etc

I'd love to create more PRs, sometimes a PR is less work than a complicated bug report, and in case of code structure improvements a PR could be clearer as well. You're still obviously free to not accept it, or debate the merits of any proposed code change. But currently this is complicated because of the way the code is structured.

[tsv2013]

Hi Sam! Sorry for delayed response.

As for frameworks support and implementing Svelte rendering.

We have the following concept: survey-core library incapsulates survey objects, their parameters, behavior and interaction. It's a Model from the MVVM point of view. A "survey--ui" library (survey-react-ui, survey-angular-ui and son on) contains the two other parts of MVVM - View - rendering: render function of react or template bindings (of angular, vue and others), and ViewModel class. ViewModel class in our concept is a thin object doesn't contain any model logic and acts like an adapter between Model and View (rendering layer).

A ViewModel objects contain some code adapting Model to framework-specific API. And as a main function in our product, ViewModel implements platform-specific reactivity to make framework react on model changes and reflect these changes in rendered UI.

I've checked Svelte reactivity documentation (https://sveltesociety.dev/recipes/svelte-language-fundamentals/reactivity). And from that I've understood, it can be used in a declarative way for implementing surveyjs ViewModel-View layers. If a Svelte component will get a survey object model (like Page or Question) it can define the properties that should be reactive using e.g. deconstruction syntax:

$: ({ one, two, three } = some_obj);

I hope a svelte implementation for surveyjs can use our general approach: pass survey model object to a svelte component as a parameter and implement "native Svelte" reactivity for the properties used in UI rendering. In any case it will need investigation and work. And in this approach right now I don't see any points to make dramatic changes in our "survey-core" library.

Even if such changes definitely will improve architecture (and code smell :-) ) they can unexpectedly hit our products stability and make many of our customers unhappy and frustrated. That's why these architecture changes will always be scrupulously checked and only after that accepted. I hope you understand our point here.

And in a situation of limitedad resources we don't want to make some abstract improvements, just for architecture improment. We want every change to bring some value for our users.

[SamMousa]

The Svelte point is not so much about the Svelte framework, but more about the output. Svelte compiles to pure JS, so the result is a framework-agnostic rendering system. This then only requires a wrapper for each frontend (runtime) framework.

Of course I realize that architectural changes don't happen overnight, but this is all the more reason to have a vision documented somewhere. A documented vision / plan allows for gradual improvements.

Things like globals are in my opinion a big issue. Take for example the "global survey settings":

There is no practical reason why these options need to be set globally / per page. If someone wants to reuse defaults that is easily done in every framework.

So why not instead of things like this:

// User code
import { settings } from "survey-core";

settings.settingName = "value";


// Survey JS code somewhere deep in the object hierarchy
import { settings } from "survey-core";
if (settings.someSetting === true) {
 // do special stuff
}

Just use normal configuration objects:

import { Settings } from "survey-core";

const myDefaultSettings = new Settings();
myDefaultSettings.someSetting = false;
myDefaultSettings.someOtherSetting = true;

const myOtherSettings = myDefaultSettings.clone();
myOtherSettings.someSetting = true;

const s1 = new Survey({}, myOtherSettings);
const s1 = new Survey({}, myDefaultSettings);

This will bring direct benefit to end users because they can now have survey specific settings instead of global survey settings. Reusability is trivially managed outside the library by just having global objects, but at least then they're managed by application code. If 50 different files in a project import settings and write to them there is no way to tell what the order of the files is since compilation tools decide this order on their own. And how can I even know for sure that the surveyJS core has not already evaluated some property of the global settings object before I've put my desired value in it...

Reactive properties

Reactive properties are managed by all renderers seperately. This is currently not working in all cases when loading from JSON for example causes bugs in the Knockout renderer, but not the Angular renderer: https://github.com/surveyjs/survey-library/issues/7168

I think all or most of this could be more simply managed using the observable pattern, as implemented in RxJs. The nice thing is that all frontend frameworks will cleanly integrate with RxJs observables:

• https://angular.io/guide/rx-library
• https://vuejs.org/guide/extras/reactivity-in-depth.html#rxjs
• https://react-rxjs.org/

In this case repeated patterns in each framework could be replaced with one implementation.

Split runtime state and survey definition

Currently the definition of the survey, as well as its runtime state live in the same object. Then the renderer also lives in the same object (but is separated in the code by a subclass).

A cleaner design could be something like this:

// This contains the survey definition, it has no runtime information.
const survey = new SurveyModel({pages: ...});

// This manages the runtime state of the survey (things like currentpage, data, currently visible questions etc)
const runtime = new SurveyRuntime(survey);

// This does the rendering in framework specific code, not sure how this looks for every framework.
<SurveyRenderer {runtime}/>

I realize we won't be separating survey from runtime any time soon, this is just to think of an ideal design. We could however separate the runtime from the renderer relatively easily. Especially if we have some observables exposed.

The advantage of using composition instead of inheritance, especially when it come to the framework implementation, is that it creates a clean API boundary. The survey runtime model needs to expose in a convenient way the things that are relevant for rendering implementations.

https://en.wikipedia.org/wiki/Composition_over_inheritance

Anyway, just thoughts not sure how to act on them in an incremental way.

[tsv2013]

Short story long

Things like globals are in my opinion a big issue. I agree that it actually is a big issue. And our team understands it. But meanwhile could you tell us - what is the exact problem you face because of a static objects in surveyjs library? Many users are using these static variables and if we get rid of them, then we'll get a huge breaking change. Right now we're discussing the shift to V2.0 from 1.9.xxx and we would like to make all BC we can. Statics are not treated as showstoppers at this moment. Probably your arguments will change opur mind.

Reactive properties Right now we're using built-in reactivity of each framework. And we don't see any reasons why we need to add a reference to RxJS

Split runtime state and survey definition The same here - what are reasons why we need to split survey runtime/definition? SurveyJS is designed in MVVM paradigm. Model in this case holds the current state and defintion doesn't look like a necessary thing. I don't think that additional objects will make design cleaner. They will be cut by Occam's razor. :-)

[SamMousa]

what is the exact problem you face because of a static objects in surveyjs library?

I listed a few of them in the initial post, the second one is very concrete: multiple creators (or surveys) with different localizations. Note that I'm not looking for, or expecting you, to fix this specific use case. I actually in practice seldomly use multiple surveys let alone multiple creators on one page.

Another one specialChoicesOrder in the global settings object, forcing me to have the same order for special item options in every survey.

[SamMousa]

Okay, a very concrete issue breaking our builds:

I've edited survey.core.js to debug some output:

    CustomWidgetCollection.Instance = new CustomWidgetCollection();
    console.log(CustomWidgetCollection.Instance);
    console.warn('nooo');

    return CustomWidgetCollection;

This outputs some stuff to the console so that I know the CustomWidgetCollection singleton has been initialized.

When building with vite this is my output:

CustomWidgetCollection {
  widgetsValues: [],
  widgetsActivatedBy: {},
  onCustomWidgetAdded: Event {}
}
nooo
TypeError: Cannot read properties of undefined (reading 'Instance')
    at registerAll (file:///projects/example/.svelte-kit/output/server/chunks/authorizedFetch.js:1581:57)

So my code that uses the instance to register a custom question type cannot find the instance during compilation time. This class of issue is very hard to debug and may be caused by any configuration setting in the environment.

[tsv2013]

Looks weird for me. Referenced libraries code is definitely shouldn't be executed in compile time. I can assume it's ф Svelte feature...

[SamMousa]

It could be; but that's not the point. Because SurveyJS has all these globals and side effects this happens. Modern code using ES modules don't suffer from these issues with any framework.

[tsv2013]

We're planning to support ES modules in this year. At least this task will be added into our 2024 roadmap.

Asd for statics - I do agree that this is a bad practice. But we don't want to do such a dramatic breaking changes right now.

As for executing code during compilation - I can't agree with you here - I believe this is a bad practice too.

[SamMousa]

As for executing code during compilation - I can't agree with you here - I believe this is a bad practice too.

I'm not sure if it's always bad practice. This is about server side prerendering and those types of optimizations. During compilation a lot of stuff is done and sometimes (as evidenced by the issues I was facing) some tool in the toolchain makes a bad decision because it is not detecting what can or cannot be prerendered. For example if your code looks like this:

const delay = 24 * 3600 * 7; // delay by 1 week

It would be reasonable for optimizers to to optimize this to:

const delay = 604800

In my case the bundler decides in what order to include the code and then probably tries to prerender it which doesn't work if it makes incorrect assumptions about the way stuff is initialized.

Anyway, cool that you're working on it!

Also there is a way forward internally to reduce globals while still offering them for backward compatibility purposes. You won't directly experience all benefits, but the advantage is that you can improve the code and when you do decide to break BC it's not a huge change in the whole code base. Anyway, it is what it is, looking forward to ESM!