search

xwp / wp-js-widgets

JS Widgets plugin for WordPress
https://wordpress.org/plugins/js-widgets/
40 stars 6 forks source link

Add JavaScript unit tests and refactor #35

Closed sirbrillig closed 7 years ago

sirbrillig commented 7 years ago

Add basic JavaScript unit tests for wp.widgets.Form. They can be run using npm test on the command-line (you may need to run npm install first to get the tests properly set up).

This also makes some changes to the wp.widgets.Form class to make the code more concise and more testable. There are also a few bugs that are fixed. Changes include:

This needs to be tested with a real-world Form to be sure that the above changes do not break anything.

There's also more simplifications that could be made, but I wanted to get a PR up for the purposes of discussion.

sirbrillig commented 7 years ago

@westonruter Another question I had, regarding the code under test: Is it a good idea to allow adding arbitrary properties to the object via the constructor argument? It seems a little bit like mixing two inheritance mechanisms in one place: the Form can be subclassed, but also you can override its properties and methods using the object passed in the constructor like a mixin.

var form = new Form({ 
    foo: 'bar', 
    sanitize: function( val ) { return _.extend({}, val, {hello: 'world'}); } 
} );
form.foo; // 'bar'
form.sanitize( {} ); // {hello: 'world'}
westonruter commented 7 years ago

@sirbrillig good question. It's probably not good form to do that. What do you suggest? Mark certain properties as being protected and throw Error if they are attempted to be overridden? Or some other refactoring of Form?

sirbrillig commented 7 years ago

@westonruter hm... I guess my first question is: what is the "normal" or "expected" use-case of that feature? That is, what are the intended arguments to the constructor?

I'll make a guess (but I could definitely be missing something): it's intended to allow setting/overriding id_base, model, container, and (certain properties of) config. If that's true, then I would say to just copy over those properties explicitly, and silently ignore any other properties in the parameter object.

I could see the benefit of throwing if there's an unexpected property, in that it might make it easier to find bugs or typos, but I could also imagine a situation where the argument object is constructed by something else and passed into the constructor programmatically and we might want to just ignore unexpected properties. Also, the constructor does already throw if the required properties are not present.

So I could go either way, but my instinct would be to ignore unexpected properties and explicitly assign expected ones. What do you think?

westonruter commented 7 years ago

So I could go either way, but my instinct would be to ignore unexpected properties and explicitly assign expected ones. What do you think?

@sirbrillig I think you're right. I think I initially was allowing anything to be passed in because it wasn't used enough to see clearly what the usage patterns should be. Really, the only params that absolutely would need to be passed in which are unique to each instance would be model and container. The id_base, default_instance, and other config parameters really should be defined on the subclass prototype itself.

So then for PHP should export its config once. And actually, this is already the case for the core widgets that are implemented: https://github.com/xwp/wp-js-widgets/blob/38d8298528737add34594efdb4db8d649355a140/php/class-wp-adapter-js-widget.php#L85-L89

And looking at the existing usages for admin screen and customizer pane, only the model and container properties are being used:

So yeah, let's update Form to only honor passing a model and container args, and we can also let Form be marked as @abstract to then also throw errors if initialize is called and this doesn't have id_base or default_config inherited from the prototype.

Does that make sense?

sirbrillig commented 7 years ago

@westonruter that makes sense. I added tests to that effect and then changed the constructor.

Another thing I just noticed (which might not actually be relevant) is that because Form overrides its model.validate method with a closure that includes methods on the Form, the model will keep a reference to that Form even after the Form is destroyed. I don't know where the model comes from or if it lives beyond the Form's existence, but maybe we should restore the original validate method in destruct?

Alternatively, if this is a concern, instead of overriding the model.validate method, we could just run our own validation in the form change event handler before the data is sent to the model. Fewer mutations of external objects would reduce the chance for errors to creep in. That is, unless it's expected that the model might receive data from another source which would require the modified validation.

I added a test to show the memory leak in case we want to change anything later.

westonruter commented 7 years ago

@sirbrillig:

Another thing I just noticed (which might not actually be relevant) is that because Form overrides its model.validate method with a closure that includes methods on the Form, the model will keep a reference to that Form even after the Form is destroyed. I don't know where the model comes from or if it lives beyond the Form's existence, but maybe we should restore the original validate method in destruct?

Oh yeah, that's a good point! Yeah, the model can indeed live beyond the Form's existence and so restoring the original validate method should indeed be done in the form is destructed.

But actually, it would probably be even better to create a new Value instance inside the Form's constructor and then to sync this new value with the supplied model. This new Value instance could then have its own unique validate method, and when the Form is destructed it can just be unsync'ed and destroyed.

So inside of initialize:

form.model = new api.Value();
form.model.validate = function( value ) {
    // Current logic in our overridden validate method.
    return value;
};
form.model.set( args.model.get );
form._externalModel = args.model;
form.model.sync( form._externalModel );

And then in destruct:

form.model.unsync( form._externalModel );
form.model = null;

This approach will keep the original args.model object pristine and it will also prevent a memory leak.

Alternatively, if this is a concern, instead of overriding the model.validate method, we could just run our own validation in the form change event handler before the data is sent to the model. Fewer mutations of external objects would reduce the chance for errors to creep in. That is, unless it's expected that the model might receive data from another source which would require the modified validation.

In this case it would just mean that the form should be forbidden from directly accessing this.model, right? Subclasses would be restricted to only using form.getValue(). This would make sense to me actually. And then the form.setState() method would then be what implements the validate logic? The model shouldn't be expected to be modified from another place outside of the Form, or if it is, we can assume that it is already validated (for example, populating the customizer settings with a previous revision).

sirbrillig commented 7 years ago

In this case it would just mean that the form should be forbidden from directly accessing this.model, right? Subclasses would be restricted to only using form.getValue().

Right. I think I like this approach the most because it's just less complexity (and one less thing for the subclass to "know about"), even though the model duplication/sync/unsync approach would certainly work.

westonruter commented 7 years ago

@sirbrillig ok, sounds good! You want to incorporate that into this PR?

sirbrillig commented 7 years ago

@westonruter I'm fine to do that unless you'd like to keep the change separate?

sirbrillig commented 7 years ago

As part of the changes I simplified two-way data binding a bit (see 16f9762). I'm not sure if the resulting simplified version covers everything that the sync version did. The primary difference is that we're only listening to the change event, rather than propertychange, keyup, and input. There's lots of other ways to sync the data so if this doesn't work we can try something else.

westonruter commented 7 years ago

@sirbrillig oh, I didn't consider how createSyncedPropertyValue related here. A problem with change is that it will only trigger when the user has blurred the field, resulting in a poorer preview experience. This is what wp.customize.Element is supposed to abstract away, using the appropriate DOM events to listen for changes to the underlying input.

I think that createSyncedPropertyValue needs to be restored, but with a change to how sync is done. Inside of propertyChangeListener we can do this change instead:

--- a/js/widget-form.js
+++ b/js/widget-form.js
@@ -272,15 +272,15 @@ wp.widgets.Form = (function( api, $ ) {
         * @returns {object} Property value instance.
         */
        createSyncedPropertyValue: function createSyncedPropertyValue( root, property ) {
-           var propertyValue, rootChangeListener, propertyChangeListener;
+           var form = this, propertyValue, rootChangeListener, propertyChangeListener;

            propertyValue = new api.Value( root.get()[ property ] );

            // Sync changes to the property back to the root value.
            propertyChangeListener = function( newPropertyValue ) {
-               var rootValue = _.clone( root.get() );
-               rootValue[ property ] = newPropertyValue;
-               root.set( rootValue );
+               var newState = {};
+               newState[ property ] = newPropertyValue;
+               form.setState( newState );
            };
            propertyValue.bind( propertyChangeListener );

This will ensure that the model is not directly mutated so that our validate logic will still apply.

Does that make sense?

sirbrillig commented 7 years ago

Ah, nice. Don't know why I didn't see that part of the function before. I guess I wasn't clear what was going on in there. I made the changes you suggested and the tests all pass. 👍 (I rebased away the old version.)

One last question:

In renderNotificationsToContainer, this command is run to change the CSS of the notifications container after it is shown: container.css( 'height', 'auto' );. I'm curious why that's only done when the container is shown and it's not changed when it's hidden? Could the height just be part of the actual CSS for the Form instead of being set with javascript? I guess that also makes me wonder: why not do the animation with CSS instead of jQuery?

westonruter commented 7 years ago

Ah, nice. Don't know why I didn't see that part of the function before. I guess I wasn't clear what was going on in there. I made the changes you suggested and the tests all pass.

Great!

I'm curious why that's only done when the container is shown and it's not changed when it's hidden? Could the height just be part of the actual CSS for the Form instead of being set with javascript? I guess that also makes me wonder: why not do the animation with CSS instead of jQuery?

@sirbrillig Humm, I don't have a good answer for you. I can't remember why height:auto was added 😦 If you see no good reason for it, the I'd say to go ahead and remove. The styling/animation logic here is copied from wp.customize.Control#renderNotifications. If you have a CSS solution then more power to you! But I think that CSS transitions/animations weren't used because they are not supported by IE<11.

sirbrillig commented 7 years ago

TIL that CSS transitions aren't available in IE < 10. It's amazing how quickly I've come to think of them as natural. Well, I don't know why height: auto is in there either, but it's a relatively minor thing either way. I'd be inclined to just remove it and re-add it if/when it becomes apparent that there's a problem that needs to be solved.

westonruter commented 7 years ago

Great! I'll give this PR a test soon and see if there's any issues on my end and then merge. Excellent work here. Really appreciate the depth you went and the insights you have.

westonruter commented 7 years ago

@sirbrillig I found that the widgets weren't getting id_base defined on the widget prototypes, so I added 5d7ccb0. This means a widget should be defined via:

wp.widgets.formConstructor.foo = wp.widgets.Form.extend({
     id_base: 'foo'
});

So there is some redundancy there. In reality they should never be different, so it seems a shame to duplicate.

sirbrillig commented 7 years ago

So there is some redundancy there. In reality they should never be different, so it seems a shame to duplicate.

I'm not sure I understand. Do you mean, the id_base property should always be the same as the name of the property or variable to which that object is assigned?

So const HelloWidget = Form.extend( {} ) should result in HelloWidget.id_base === 'HelloWidget'?

westonruter commented 7 years ago

I'm not sure I understand. Do you mean, the id_base property should always be the same as the name of the property or variable to which that object is assigned?

sirbrillig commented 7 years ago

I removed id_base from Form. Let me know if there's anything else!