Feature request: Parameters checks and Validation

[Itee]

Next to #12231 and #12574 part 3

The main idea is to check if parameters are valid in ctor, setters and public methods in development environment. This will allow a big benefits during dev and will make a safer library. All asserts could be easily removed when building for production using: rollup-plugin-strip

The assert could use a Validator like in ObjLoader2 for common validation like: isString, isArray, isMatrix4 etc...

An example:

getWorldPosition: function ( target ) {

        assert( target !== undefined, 'THREE.Matrix4.getWorldPosition: Missing required target.' );
        assert( Validator.isVector3( target ), 'THREE.Matrix4.getWorldPosition: Invalid argument, expected THREE.Vector3.' );

	this.updateMatrixWorld( true );

	return target.setFromMatrixPosition( this.matrixWorld );

},

[Itee]

/ping @mrdoob So what is your opinion ?

[mrdoob]

If we start doing this, it'll be difficult to know where to draw the line of what to check and what not. This is basically fighting against JavaScript. I would rather move to TypeScript than going down this path...

[Itee]

I not agree with that:

This is basically fighting against JavaScript

because currently it is:

Fighting against Three because i don't know what and where it is wrong in my code. And the error i got is completely out of place. (Ex: The "position" attribute is likely to have NaN values.)

Lot of the biggest libraries in JavaScript already use this type of assert in development environment to help their developers. If the biggest think it's a good solution maybe it's a good solution ?

And i know your aversion about TypeScript so...

What can we propose to the developers to help them to know what mistake they do, and prevent them to search during hours an error that could easily be catch with a simply assert against parameters ?

I see 3-4 options:

• Add asserts like i propose, with good performance in dev, a performance gain in prod, based on a contract about which could be like null, undefined and the type.
• Use TypeScript
• Use Flow
• Do nothing and continue in this wrong way

So please, tell me what can we do ? Have you a better solution about this concern ?

[looeee]

@Itee there is a third path: expect developers to learn how to debug their code properly.

This is a vital skill and not actually that hard. Even using the most basic technique of following through the flow of your code and putting in console or debug statements will quickly lead you to the source of the error, then you can see for example that you are passing in the wrong argument to a function, or that NaN values are being generated and passed as position attributes at this point.

Of your your options:

• asserts: if this was done all over the library it would be a considerable increase in code size and complexity and I think probably a performance drop too. Are you aware of any other large JS libraries that use this approach?
• Typescript: I don't really have any strong opinion here, except that it would make three.js less accessible for non TS developers, which is most of the JS community
• Flow: This is non-trivial to set up and puts an nonviable expectation on the development environment that people use
• Do nothing and continue in this wrong way, expecting people to learn how to debug their code properly

The last one seems reasonable to me at least - although I think it should be combined with a strong effort to keep the documentation clear and up to date.

[Itee]

@looeee It's a beautiful dream ! 😄

About:

• TypeScript: i totally agree with you
• Flow: totally agree too
• People to learn how to debug: a nice dream
• asserts: If we had a simple assert for all arguments all over the library it will increase the size by maybe 1000-2000 lines (not so big, and only in dev) ? The complexity... I'm not really sure that add an assert will increase the complexity of the code, on the contrary it will clarify the intent, will allow to prove that the function do exactly what it should do using unit test with, and help the dev to understand what is his mistake !

Finally,

Even using the most basic technique of following through the flow of your code and putting in console or debug statements will quickly lead you to the source of the error

Debug with console is totally insane and time consumer ! Using the debug tool with breakpoints is better, but can take a while, and/or just be unable to use in case of unreproductible/random bug. Instead of got immediately why and where it's broken ???
At last, using assert will allow to catch tone of bug that are currently in the library but hidden (IMO: I talk about experience when upgrading base code with this type of protect pattern who raised a ton of bugs. )

Check user input on the public interfaces is not a good practice, but a need !

[looeee]

OK, although I'm wondering now why you are suggesting options that you don't agree with.

Regarding asserts, as I asked above, do you know whether any other large JS libraries use this technique to validate parameters?

[Itee]

If you want specifically about assert i don't really know, but about parameters check almost all ?!? With their own implementation using personal logger, throwing error, what ever they need to do the job !

• npm
• bower
• react
• blocky
• closure-library
• etc...

Ok before go further in the discussion, did you read this #12231 ?

Next, i will clarify some points.

why you are suggesting options that you don't agree with.

I agree with this options (except the last of course 😄), but they have pro/con like you list it previously.
TypeScript or Flow allows to check if parameters are correct in a certain way ! And will avoid some headache during development.

The assert part is IMO most powerful than the others, because it will provide a very fast feedback about what is the problem. Require very few characters to be useful. Can be deleted easily in production using rollup-plugin-strip.

My goal is (like i said in first place):

The main idea is to check if parameters are valid in ctor, setters and public methods in development environment. This will allow a big benefits during dev and will make a safer library.

The method to do it, is the question. I think assert or equivalent javascript check using a Validator are the best to do. But any options that do the job is ok for me !

But currently i want to know which option we will use to do that ! To apply it on future code refactoring.

I hope i'm clear 😟 It's not really easy to me to explain some abstract meaning due to my poor vocabulary in English, or bad sentences construction...

For example when i'm talking about asserts, it is more like any javascript line code that allow to check parameters validity and tell what's wrong to the user.

the

assert( target !== undefined, 'THREE.Matrix4.getWorldPosition: Missing required target.' )

was just an example taken from #12231

[looeee]

Ok before go further in the discussion, did you read this #12231 ?

Yes, I've read over that thread. I would say it's very far from conclusive that this won't have a large performance impact based on that thread. At the very least we would need to make certain of that before going further with this.

If you want specifically about assert i don't really know, but about parameters check almost all ?!?

Can you provide some specific examples? I just had a (very quick) look through the source of a couple of libraries (lodash, jQuery ) and it looks to me like they take very similar approach to what we do here - that is, set up default values where neccessary, but otherwise assume that the correct type of object is being passed in, except for a few cases where they are using "typeof" and sending a message to the console if the type is incorrect.

[Itee]

The performance impact will only appear on development, and if disable it will just cost a boolean check. Which have not really impact. In case you're in debug mode the impact is acceptable due to the state, aka development.

About examples:

1. npm:
   • adduser
2. bower:
   • home
   • update
3. jQuery:
   • data
4. lodash:
   • createPadding - With additional doc for IDE !

I just picked up some file randomly and find params check everywhere. Not for all params of course because it will required a more deep reading of this library. But you can be sure that all their inputs are check somewhere and could be afterware use in a safe way !

[mrdoob]

The performance impact will only appear on development

So we can't expect people to learn how to debug but we can people to know how to develop in debug mode and release mode? 🤔

I just picked up some file randomly and find params check everywhere.

You should compare this with other graphics-related libraries. The ones you picked are not so focused on performance.

[Itee]

So we can't expect people to learn how to debug but we can people to know how to develop in debug mode and release mode?

Toggling a Boolean value in constant file for example is (i hope) much easier for a beginner than run through chrome debug tool in a new and unknown big library like three ? About the debug learning... Catching error early against wrong params will, on the contrary, help a lot beginner to understand what happen in the code base, and what they do wrong, no ? That allow them to become better quickly.

You should compare this with other graphics-related libraries. The ones you picked are not so focused on performance.

I think there is something that i don't fully understand here...

What is the problem to make a relative ( to define, but tiny ) performance impact in development environment under debug mode on, and virtually no impact when off. And absolutely no impact in production environment ???

lodash i think is about performance ( but not graphical of course ).

[mrdoob]

Toggling a Boolean value in constant file

But that assumes the user is using a build system. An easier approach would be to have more builds... three.release.min.js, three.debug.min.js, ... But, either way, I guarantee you that tons of people will publish the debug version.

What is the problem to make a relative ( to define, but tiny ) performance impact in development environment under debug mode on, and virtually no impact when off. And absolutely no impact in production environment ???

The problem I have is forcing users to node/webpack/build... development workflows. I understand the benefits, but I also understand that it makes the experience less fun. It is important for me to keep the experience fun.

[looeee]

Going over your examples:

lodash.createPadding

This example is not doing type checking - it takes 2 arguments, length and chars.

If you fail to enter length it will crash and burn by the look of it - certainly if you enter a string or NaN or something instead of a number you'll get weird results.
Iif you fail to enter chars then it gets set to a default value, which is something we already do in most cases here.

jQuery.data

function getData( data ) {
	if ( data === "true" ) {
		return true;
	}

	if ( data === "false" ) {
		return false;
	}

	if ( data === "null" ) {
		return null;
	}

	// Only convert to a number if it doesn't change the string
	if ( data === +data + "" ) {
		return +data;
	}

	if ( rbrace.test( data ) ) {
		return JSON.parse( data );
	}

	return data;
}

There's no parameter checking here either - if you enter anything other than a string it's just going to get returned to you unchanged, which is probably going to cause weird errors if that's not what you expect.

NPM and Bower are not really relevant here since they are intended to be run on your local PC or server and don't have to worry about performance to the same degree.

I would agree with @mrdoob that only other libraries focused on performance to the same degree as three.js are relevant here. Although I think lodash is a good example, and it looks like it doesn't do any more checking than we do.

[Itee]

The performance is not relevant here, because in case of production it will be strictly equals to current state ! But... like said @mrdoob:

The problem I have is forcing users to node/webpack/build... development workflows.

I'm clearly for a normalized library that allow perfect consistency between each class/modules/whatever in three. That make code clearer and easier to learn/use/maintain. But i understand that can hurt some people.
About forcing the user, sorry i don't see where it forcing workflows to just allow them to debug their code only if/when/where they want ? Or maybe i missed something ?!

About:

An easier approach would be to have more builds... three.release.min.js, three.debug.min.js, ... But, either way, I guarantee you that tons of people will publish the debug version.

[irony on]
Yes, you clearly point a problem... People that need to learn how to debug in a library of hundreds of thousands code lines without any debug output about their inputs, won't be able to set the correct file in production... ( In fact this could certainly be the case, sadly ). Maybe they should learn how to do this too, no ?
[irony off]

Anyway, the fact to create new builds files for different environments could be disturbing. Hey ! Wait a minute ! We already have three.js and three.min.js... which are designed for dev and prod, no ??? Personally, i will never use a three.debug.min.js file and always use a three.release.min.js file. The intent of the minification is to be use in production, right ? So in fact, we don't need to create new builds file, and users that could publish the debug version are one of them that currently publish three.js in production. So that change anything !

But may be we should take 2min to analyse what could be the impact to the end user building process. (*1)

About the fun, yes i agree that coding need to be fun ! But, debugging during hours and fighting against a big library like three when you're learning it, is IMO not so fun. On the contrary a beginner that can easily write and debug his code will got more and faster fun about his 3d application. As far i known three is intended to be the more generically usable possible for every people, and not only for developers. So just take 2scd for this short user story:

A webdesigner, named John, need to create a website including 3d. He never wrote a code line of his life. He ear about the great three webgl library. He want use it, happily for him there is a documentation with some starter examples. But a day, when he copy paste an example, he need to make some changes and OMG something went wrong... He isn't aware about his browser debug tool, and start an epic quest to find what is wrong with the code... After googling on stackoverflow and learn that he can type f12 to get console debug output he see... an error ! Sadly this error is absolutely not related to his real problem. The website did not arrive on time, he was fired. Loosing his job, his wife leave him. He become homeless, and finally committed suicide by jumping off a bridge...

This is cruel to don't help this type of people @mrdoob ! If the debug error was related to the real problem John would still be alive !!!

So maybe am i wrong with this type of features. But during my life, using almost all programming language i always see and ear that users inputs must be checked. For the simple principle of GIGO.

@looeee Maybe i don't take best examples, but the idea is they check user inputs. And maybe this is a bias from my experience but check inputs params avoid tons of bugs for the library and for the end user. Finally, like i said the performance is not a relevant topic here because production won't embed any debug stuff. My goal is just to make three more robust, usable and easier for beginner. And i hope/sure you share the same goal.

tl;nr -----------------------------

1: Here i will try to list and develops some users building process using three to see if this type of argument checking, coupling to a global variable (named here DEBUG_THREE) to allow/disallow debug stuff, is relevant or not.

1. Using script tag to import the all library from html:

<script src="../build/three.js"></script>
<script>
    var foo = new THREE.Vector3();
</script>

This is the development file, so the own building process of Three had already set the global variable DEBUG_THREE to true. The user will be clearly warn about error he can do.

<script src="../build/three.min.js"></script>
<script>
    var foo = new THREE.Vector3();
</script>

This is the production file, so the own building process of Three had already set the global variable DEBUG_THREE to false. The user won't be clearly warn about error he can do, except by browser error that are not always relevant to the main problem.

The impact is only in Three repository, that need to set correctly a variable depending on... Maybe on NODE.env ?

2. Building his own three

The developer just need to take care to submit the NODE.env variable to the building process. And i'm sure that can be automated using package script arguments.

3. Bundling using three.module.js:

The three.module.js was copied in an assets folder, and developer relies on it using es6 import statement. He can toggling manually the DEBUG_THREE variable include in the copied file. Or submit NODE.env if DEBUG_THREE is declared with something like:

DEBUG_THREE = NODE.env.dev || false;

4. Bundling using node_module/three

The devloper can't toggling manually the DEBUG_THREE variable, but can still submit NODE.env if DEBUG_THREE is declared with something like:

DEBUG_THREE = NODE.env.dev || false;

In that way his final build will take care of his environement.

5. Using webpack

I'm not aware about the problematic of sharing an global variable from a third_party ( three ) library using webpack. Sorry ! If someone can't tell me the pro/con ?

[looeee]

Holy wall of text, batman!

Do you really expect people to read all of that?

But focusing on your response to my point above:

Maybe i don't take best examples, but the idea is they check user inputs

In your examples, they don't validate function parameters.

[Itee]

I hope they will ! Because i explain lot of things about this concern. And personally i take the time to read some block of text when the topic is interesting. But i can split this response in 4-5 comment more digest, but the size will be the same.

they don't validate function parameters.

So, they should ! 😄

But to the contrary, why should we not validate function parameters ? What are the gain ?

[looeee]

I hope they will

The problem is, that no matter how much you hope, they really won't. Personally I got to the story about Johnny web developer and his divorced wife and gave up.

But anyway, back on topic:

This would be quite a major change to the library, and I'm not convinced that it's neccessary - especially since, from the examples of other libraries we have seen so far, the level and type of validation we are currently doing seems pretty standard in JavaScript.

But if we do go ahead, we'd need to make sure at least

• that this doesn't favour any particular way of using the library - nothing specific to node, webpack, rollup, including via script tags or imports statements or anything else (so no NODE.env or other environment variables).
• must be beginner friendly
• minimal performance impact in dev
• very close to zero impact in production
• minimal impact on code complexity

The only way I see to satisfy all of these would be to add an extra build target three.debug.js - and I don't think it's unreasonable to expect people to switch to three.min.js in production. I'm not sure how this would work for people that are using build tools though.

[Itee]

Under tl;nr is just extra stuff that is not 'required'. And i rarely make so big text in fact because it take lot of time to me. I will be more concise now. But thank for reading !

This would be quite a major change to the library

Yes

I'm not convinced that it's neccessary

I am, but the real gain will come after the implementation.

currently doing seems pretty standard in JavaScript

We can do better than standard

that this doesn't favour any particular way of using the library - nothing specific to node, webpack, rollup

Thank to point this, you're totally right ! I will think about a non specific way to achieve this.

must be beginner friendly

That the purpose ! To help beginner to debug easily their code in fast and lighter way than currently.

minimal performance impact in dev

Need a benchmark, but should be minimal this cost just an boolean check per methods under dev when debug off.

very close to zero impact in production

It is not very close, it is zero impact due to the removal of the code line during bundling using rollup-plugin-strip (if we go this way)

minimal impact on code complexity

This will improve code readability ! In that way you will be able to know which type of the param should be, and you will be sure that you cannot get crappy undefined or null values ( if the param is required of course ! )

an extra build target three.debug.js

Why ? What is the purpose of three.js and three.min.js if it is not for dev (aka debug) and prod env ??? For fun ?

I don't think it's unreasonable to expect people to switch to three.min.js in production

This is the only goal of this file, no ?

I'm not sure how this would work for people that are using build tools though.

What do you mean by build tools ? Rollup or webpack ? In this case see tl;nr section above.

[Mugen87]

I agree with @mrdoob and @looeee . I suggest to close this issue and focus on other topics instead.