Rework JS integration without code injection

[herrvigg]

A major rehaul of the JS part is currently going on with webpack and babel. I'm realizing now there are a few issues with the i18n-config.json:

• it allows to embed JS code directly. Code injection like that is not safe, we should remove this and have JS files clearly separated and identified.
• the integration allows js-conf and js-exec for execution before and after qTranslate respectively. No one uses js-conf it seems.
• the grain is too fine, this tends to multiply the number of executed scripts with one script per entry. When bundling an application we want rather the opposite, regrouping many parts in a single bundle. Though the script may be bigger this is usually better as it's in the cache and reduces the number of connections. Still we may have separate scripts per supported modules / plugins / themes but this could evolve.
• instead of having js-exec deciding which script to load, an idea could be to use the keys of the pages instead and it should be up to the JS scripts to do the branching internally.

I don't think many people use the JS integration since it's been so cumbersome. The main plugins are now integrated as modules and the first concerned is ACF that we can handle directly (that was the idea of taking the plugins as modules internally, allowing easier refactoring). Perhaps I'll have a phase of deprecation but i'm tempted to break the js-exec config entry.

Any comments welcome.

[picasso]

I used js-exec a lot.

I didn't really understand this idea:

instead of having js-exec deciding which script to load, an idea could be to use the keys of the pages instead and it should be up to the JS scripts to do the branching internally.

could you explain?

[herrvigg]

Great, it would be interesting to see how you use it.
If you look at every entry in admin-config you have for example this:

{
 "post": {  // <---- this is the key
      "pages": {
        "post.php": "",
        "post-new.php": ""
      },
      "anchors": ...,
      "forms":  ...,
      "js-exec": {
        "post-exec": {
          "src": "./dist/post-exec.js"
        }
      }
    },
  }

The main logic behind the JSON configurations is selectors / LSB placement / fields selectors + behavior / js code. The first key identifies each block, in this case it's called post and logically the JS script is called post-exec.js.

What I dislike is that this tends to create a single JS file per entry leading to a lot of "fragmentation" with the scripts. When you bundle an app (webpack, rollout, ....) you have this fragmentation in the source files but you don't want this in the target bundle(s). Also code injection with javascript entry is quite terrible. I dislike seeing JS code in a JSON file and potentially this could be unsafe on certain environments. It's quite convenient for short scripting but this is more of a hacky solution.

My idea is doing the branching according to these keys (or IDs) directly in JS instead. They are not passed to the client part yet. Currently we have qTranslateConfig.page_config containing anchors, forms, pages ​but we could add something for these keys.

Fo​r example it could be:

if ('post' in qTranslateConfig.page_config.keys)
{
  // call the execution for previous post-exec.js
  do_post_exec();
}
if ('edit-tag' in qTranslateConfig.page_config.keys)
{
  // call the execution for previous edit-tag-exec.js
  do_edit_tag_exec();
}
// and so on...

Maybe there's even a better solution with events or other. But the main goal is to reduce the number of scripts, remove the code injection and move the decision making to the JS scripts. Also this would simplify the i18n-config.json.

A question remains for the main script to load. It can be standardized, for example qtranslate.js with possibility to override this somewhere. In some cases we may still want several scripts to load, but that special cases should be up to the plugin/theme to deal with.

[herrvigg]

Also note you may have different entries (page configs) linked to the current page, due to overlapping selectors so it's not necessarily a single key. This fusion comes from qtranxf_merge_config in PHP.

[picasso]

You are right. Now my JSON looks like this:

"post":{
	"pages":{
		"post.php":"", 
		"post-new.php":"", 
		"term.php":"", 
		"edit-tags.php":"action=edit"
	},
	"anchors":{"post-body-content":{"where":"first"}},

	"forms":{
		"post":{
			"fields":{
				"wpcf-start-point":{ "jquery": "#wpcf-group-event-attributes input[data-wpt-id='wpcf-start-point']"}
			}
		}
	},
	
	"js-exec": {
		"tplus-post-exec" : {
		    "src": "plugins/translate-plus/js/tplus-admin-posteditor.min.js"
	 	}
	}
}
,

"edit":{
 	"pages":{
		"edit.php": ""
	},
    "anchors": {
        "posts-filter": {
            "where": "first"
        }
    },

	"forms":{		
		"posts-filter":{
			"fields":{
				"column-title":{ 
					"jquery": ".column-title.page-title .hidden .post_title",
					"encode": "display"
				},
				"tags":{ 
					"jquery": ".tags.column-tags a",
					"encode": "display"
				},
				"focuskw":{ 
					"jquery": ".wpseo-focuskw.column-wpseo-focuskw",
					"encode": "display"
				},
				"metadesc":{ 
					"jquery": ".wpseo-metadesc.column-wpseo-metadesc",
					"encode": "display"
				},
				"qtx-seo":{ 
					"jquery": ".column-qtx_seo span",
					"encode": "display"
				},
				"comments-view":{ 
					"jquery": ".comments-view-item-link",
					"encode": "display"
				},
				"notice-b":{ 
					"jquery": ".notice b",
					"encode": "display"
				}
			}
		}
	},

	"js-exec": {
		"tplus-edit-exec" : {
		    "src": "plugins/translate-plus/js/tplus-admin-edit.min.js"
	 	}
	}
}

,

"widgets": {
    "pages": {
        "widgets.php": ""
    },
    "forms": {
        "widgets-right": {
            "fields": {
                "widget-text_title": {
                    "jquery": ".text-widget-fields input.title"
                },
                "widget-text_text": {
                    "jquery": ".text-widget-fields textarea.text"
                },
                "widget-custom_html-content": {
                    "jquery": "textarea[id^='widget-custom_html-'][id$='-content']"
                }
            }
        }
    },
    
	"js-exec": {
		"tplus-widgets-exec" : {
		    "src": "plugins/translate-plus/js/tplus-admin-widgets.min.js"
	 	}
	}
}

,

"upload": {
    "pages": {
        "upload.php": ""
    },
    
	"js-exec": {
		"tplus-upload-exec" : {
		    "src": "plugins/translate-plus/js/tplus-admin-upload.min.js"
	 }
	}
}

that is, for each page its own JS file is loaded.

I got your idea. I'm also prefer the idea of having one JS file instead of 4. As for identification, I suggest WP terminology. For the action admin_enqueue_scripts, we get the $hook argument that points to the page. You can also call the new key - hook

[herrvigg]

@picasso I think I found a pretty good solution that should generalize well, see PR #1009.

[herrvigg]

Changes merged in master regarding the configuration files and integration with JS code:

• introduce new events (Refactor 'js-exec' config entries with JS events #1009) for better JS integration: this allows to unify the scripts
• deprecate javacscript in js-exec: no more embedded code
• deprecate js-conf. I could not really see any relevant use case so I propose to suppress this and come back to the real needs.

Note these keys are just deprecated but they still work the same except there will be a warning raised in the admin panels. These keys will be removed in future versions so this gives us time to adjust any problem.

I believe the new approach is a better design, safer and giving a structure, still flexible but with safeguards.
We might still need something to load the JS scripts themselves for basic cases (integration of themes and plugins) but that should remain very simple (possibly a single script).

[herrvigg]

@picasso I reverted the deprecation of js-exec but I deprecate javascript sub-key (under js-exec) and the js-conf entries. Deprecation means it still works as before but a warning will be emitted when saving the options. This will be removed completely in the future.

It turns out it was much more complicated than expected to replace js-exec with something new. The admin-config structure is quite risky to manipulate and adding new options would add unnecessary complications. A better JSON design would require radical changes in a major version and breaking many existing things. For now I remain more conservative, there is already a lot that can be improved by unifying JS scripts thanks to the new events.

[herrvigg]

Changes discussed above released in 3.10.0.

[picasso]

@herrvigg sorry for the delay in answering, I was on a business trip.

I think I found a pretty good solution that should generalize well, see PR #1009.

Good idea, I like it.

It turns out it was much more complicated than expected to replace js-exec with something new. The admin-config structure is quite risky to manipulate and adding new options would add unnecessary complications. A better JSON design would require radical changes in a major version and breaking many existing things. For now I remain more conservative, there is already a lot that can be improved by unifying JS scripts thanks to the new events.

That is, you were unable to use the events?
My immediate plans are to refactor the plugin where it was used and maybe I will offer some options when I start working on it.

[herrvigg]

The events work fine! They are already in use in QTX's own scripts for example:

qtranslate-xt/admin/js/pages/post.js

Line 15 in a49f284

$(document).on('qtxLoadAdmin:post', (event, qtx) => {

I still kept the js-exec entry to allow declaring the scripts to be loaded (queued after the main QTX script).

So you could definitely regroup all your JS code into a single script (as a final bundle or a conventional source file).

But I also realized there could be some very nasty conflicts when integrating JSON files, see #1013. That's not a new problem with the JS refactoring, it's always been the case.

Seeing the example of JSON you provided, I recommend you prefix all the main page keys with a prefix corresponding to your plugin, theme or any identifier.

For example:

"mycomponent-post": ...
"mycomponent-edit": ...
"mycomponent-widgets": ...
"mycomponent-upload": ...

Doing so you ensure there won't be any overwriting with the native configuration of QTX.
You'd then declare the handlers as:

$(document).on('qtxLoadAdmin:mycomponent-post', function (event, qtx) { <your code> });

$(document).on('qtxLoadAdmin:mycomponent-edit', ...

In the future we'd need to find a better solution to prevent this kind of JSON conflicts but for now the simplest is to do like this.

[picasso]

Seeing the example of JSON you provided, I recommend you prefix all the main page keys with a prefix corresponding to your plugin, theme or any identifier.

Ok, I see. Although, if it were possible to specify the prefix once (but where?) for all pluggable configurations, it would be easier to work with and manage. Well, this is my wish. I know from my expirience that if the prefix needs to be repeated more than 10 times, then it will definitely get lost in one place and this will lead to a nightmare when debugging later.