Expose getNestedEditable and is* widget helper functions

[Piotrek Koszuliński]

They are useful also for widgets definitions, so they should not be private.

[Artur Delura]

I think we should expose constructors in ​such way. I see that we don't have standard for that.

• We are exposing constructor by passing it as field which start from lowercase letter.
• I like the idea how it's done in undo plugin - first we create object literal and then add to it constructors as a property which name is exactly the same as contructor.

Changes in ​branch:t/12150, but I think we should also make changes listed above.

[Piotrek Koszuliński]

We have a standard that says - constructors in the public API are lowercased. You can find this everywhere except undo plugin. So in this case the undo plugin is incorrect and moreover, what the undo plugin exposes is private, so we could change that, but what the widget plugin exposes is public so we can't touch this.

[Piotrek Koszuliński]

Since we are adding methods to public API we need at least basic tests for them.

Moreover:

• @returns {CKEDITOR.dom.element|null} Element - this notation is incorrect - null is not a type. Null is "not an object". Usually we explain in the textual part of returned value's description that null may be returned. For example: "Widget wrapper or null if wrapper was not found.".
• The newly defined methods should be moved to the right place - this can be e.g. right after Widget class prototype. Now they are surrounded by private stuff.

[Artur Delura]

Replying to Reinmar:

• @returns {CKEDITOR.dom.element|null} Element - this notation is incorrect - null is not a type. Null is "not an object". Usually we explain in the textual part of returned value's description that null may be returned. For example: "Widget wrapper or null if wrapper was not found.".

Why null is not a type? Why null is not an object? And finally: Why overcomplicate things and not keep them semantic? I know that there is no constructor for null, but AFAIK JSDuck support values in @param, @return etc. See ​here.

[Piotr Jasiun]

Agree. CKEDITOR.dom.element|null} is far more readable then description. On the other hand if we use one convention we should stick to it.

[Piotrek Koszuliński]

Hmm... You're right. I must have remembered this incorrectly.

So - I'm ok with null in the type of returned value, but remember that we use /, not |. And still it's sometimes good to explain when null may be returned (not in this case though).

[Artur Delura]

Replying to Reinmar:

Hmm... You're right. I must have remembered this incorrectly.

So - I'm ok with null in the type of returned value, but remember that we use /, not |. And still it's sometimes good to explain when null may be returned (not in this case though).

Added extra commit with fixes. Do we need test for is* methods? They are simple as hell, I dunno. Changes in ​branch:t/12150.

[Piotrek Koszuliński]

There should be at least one test per method.

[Artur Delura]

I added extra tests. Plase don't check jscs because they will be fixed directly in major. Changes in ​branch:t/12150.

[Piotrek Koszuliński]

Plase don't check jscs because they will be fixed directly in major.

I don't understand. Major is clean, so the branch should be clean too.

[Artur Delura]

Major is dirty, but I'll handle all so I'm getting off from review for a while.

[Piotr Jasiun]

I measured how big is the file size increase of minified widgets plugin.js and minified ckeditor.js build with the widget plugin, before and after changes:

	branch	major	diff	% increase
plugins.js	28649	28041	608	2
plugins.js.gz	9314	9238	76	0.8
ckeditor.js	559108	558481	627	0.11
ckeditor.js.gz	166127	166019	108	0.065

Change is not very big, but making methods public makes editor package definitely bigger.

[Piotrek Koszuliński]

Very interesting analysis. One more thing that I'm curious is what would be the result if in the widget plugin we used these functions via their names (as isXxx()) instead by using them as Widget.isXxx() and we exported them to the Widget constructor in one place.

[Piotr Jasiun]

Anyway R- because of ​this test. Generally it is bad practice to create big tests. If such tests fail, you do not know what goes wrong and need to learn how the whole test works. Tests should be as small and as simple as possible. One test should check only one thing. Especially in this case when you have multiple isTrue/isFalse assertions without descriptions, so if one of then will fail you will have no idea what is wrong.

[Piotrek Koszuliński]

BTW. I noticed that API documentation needs to be fixed.

1. Order of tags is incorrect.
2. The @member tag does not have to be specified.
3. Some descriptions are missing.
4. In few places I found {boolean}. The type is {Boolean}.

[Piotr Jasiun]

Also ​this commit should be squashed with the previous one with introduce these changes.

[Artur Delura]

Replying to pjasiun:

Anyway R- because of ​this test. Generally it is bad practice to create big tests. If such tests fail, you do not know what goes wrong and need to learn how the whole test works.

Generally speaking yes, but this is test for static functions. Each part is independent, I think this test need more comments (each assert might have a message what is expected, and DOM elements might be described) rather than split into parts.

[Piotrek Koszuliński]

Generally speaking yes, but this is test for static functions. Each part is independent, I think this test need more comments (each assert might have a message what is expected, and DOM elements might be described) rather than split into parts.

This test should be split into far more than 2 test. Basically - one test per one method. You should avoid testing more than one thing at a time. Sometimes it's hard, but this time it's simple.

[Piotrek Koszuliński]

I pushed two fixes to ​branch:t/12150:

1. First adds docs.
2. Second hides two methods which have no value for other developers.

[Piotr Jasiun]

Tests are still not good enough.

Firstly I have no idea what ​this test do. Why the first and third assertion should be false, and the second true? I have also no simple why to check if the method is covered properly. If such test fail I will not know which assertion is red and why. The same about the rest of the refactored tests.

The simplest solution is to add description message to assertions, but it is better to create separate tests with that description in the tests name. It is not more work and this way we would know which assertions fails (not only the first) and why.

Secondly ​parserEl1..5 are meaningless names for variables. Test would be far more readable if you would use:

assert.isFalse( Widget.isParserWidgetElement( parserElDataWidgetFalse ) );

Instead of:

assert.isFalse( Widget.isParserWidgetElement( parserEl3 ) );

Finally, we do not check if static functions exists, especially when we check if it works properly in the same test suit, so ​this test is not needed.

[Artur Delura]

Changes in ​branch:t/12150.

[Piotr Jasiun]

I rebased branch on newest major and pushed one more tests improvement. But I have one more doubt.

Would not it better to have isWidgetElement method which accept both CKEDITOR.dom.node and CKEDITOR.htmlParser.node instead of two methods? The same for is*WidgetWrapper. Our API would be simpler and build smaller. Also we could replace isDom* with just is*, because this is not clear what this Dom stands for? I understand that this is the type of the parameter and it makes sense for internal usage, but as an API it looks bad and the type of the parameter is described anyway.

[Piotrek Koszuliński]

Those functions cannot be merged without complicating them internally (it must be checked whether an argument is an instance of X or Y or none), writing more tests etc. Complication will affect performance and tests will need time. Both are things I would like to avoid, especially at this stage of working on this ticket.

[Piotrek Koszuliński]

Also, we need to remember that these are helper methods, not a first-level API which should be optimised in terms of usage and clarity.

[Piotr Jasiun]

Ok. So I closed this ticket as it is. ​git:96d5130