New OutputFilter: <script type="module">

[lkraav]

Is your feature request related to a problem? Please describe.
ES Modules are a growing piece of the ecosystem. https://core.trac.wordpress.org/ticket/12009 still doesn't have a core solution.

Describe the solution you'd like
Add new OutputFilter here.

Describe alternatives you've considered
N/A

Additional context
N/A

[Chrico]

Thanks a lot for your input. I need a bit time to check this. I'm not so sure if an OutputFilter is the correct place to implement this. For me this is more a kind of "variation", so it should be configurable for example on the Script itself like Script::withType(Script::TYPE_MODULE) or maybe we can also have a new ModuleScript.

[lkraav]

For me this is more a kind of "variation", so it should be configurable for example on the Script itself like Script::withType(Script::TYPE_MODULE) or maybe we can also have a new ModuleScript.

I don't know. I'm eyeballing this package's structure, both ModuleScript and new Script::withType() would be somewhat asymmetric to how existing system works.

I've also seen add_script_add_data() used like:

global $wp_scripts;

wp_script_add_data( $handle, 'type', 'module' );

add_filter( 'script_loader_tag', static function( string $tag, string $handle ): string {
  if ( ! empty( $wp_scripts->registered[ $handle ]->extra['type'] ) ) {
    $tag = str_replace( '<script ', '<script type="' . esc_attr( $wp_scripts->registered[ $handle ]->extra['type'] ) . '" ', $tag );
  }
}

But at the end of the day, you're still with str_replace, so seems aligned with OutputFilter system here 🤔

[nlemoine]

There's also some issues with the basic str_replace solution (#7)

Note that WordPress can also output a type="text/javascript" (see #7 (comment)). I don't know how a browser would behave with both type="text/javascript" and type="text/module" 🤔

I think hardening the OutputFilter logic would be a place to start 😉

[lkraav]

I think hardening the OutputFilter logic would be a place to start win

I actually have preg_match() based logic to account for existing type attribute, but cut it out because I wasn't sure how to build a test for it. Thanks for reminding me on how type="text/javascript" can be output, this may be enough motivation to refactor it back to work with both cases.

Hardening OutputFilter logic could boil down to moving to proper DOMDocument. Are you OK with bringing this dependency in?

[nlemoine]

Hardening OutputFilter logic could boil down to moving to proper DOMDocument

That would the best way but it would not be as performant as a basic str_replace and it seems a bit overkill to me for such a task.

I started working a some ways to improve this that doesn't involve DOM parsing a while ago, I didn't have time to finish this properly. Adding the URL and/or handle (every asset has to have a unique handle -> id='my-script') in the replacement, although not bullet proof, would harden things enough IMHO.

You check it here : master...nlemoine:fix-output-filters

[saas786]

Hardening OutputFilter logic could boil down to moving to proper DOMDocument

That would the best way but it would not be as performant as a basic str_replace and it seems a bit overkill to me for such a task.

I started working a some ways to improve this that doesn't involve DOM parsing a while ago, I didn't have time to finish this properly. Adding the URL and/or handle (every asset has to have a unique handle -> id='my-script') in the replacement, although not bullet proof, would harden things enough IMHO.

You check it here : master...nlemoine:fix-output-filters

DOMDocument can be solid approach, only downside can be performance here, if we can setup a test code to verify performance for str_replace vs DOMDocument large no. of scripts / styles.

[lkraav]

I checked master...nlemoine:fix-output-filters

Couldn't determine whether it makes sense to bring in current_theme_supports() complexity directly in here.

Pushed an update to #20 to preg_match handle any existing type attribute value, with an extra test to boot.

To me eye, seems like the currently clearest and simplest available approach.

DOMDocument measurement would be interesting to run, but I'd rather work on 3rd party asset control issue. I think with DOMDocument, we'd be talking some ms difference vs str_replace, which just doesn't justify much time spend outside scientific interest.

[Chrico]

Thanks a lot for the really good activity and effort put into this issue/pull! :)

Sry to say that, but maybe i need you to ask to slow down a bit.

Latest WordPress in version 5.7 introduced a new API to print scripts and inline scripts with various attributes [1].
Sadly this API is not yet used internally for WP_Scripts::do_item(), but i can foresee, that this will be used in future, to internally print scripts. I would like to wait a bit here and maybe find some time to check https://core.trac.wordpress.org/ [2] [3] to get a good overview about the directon they are planning to go.

This would maybe allow us to just implement a new public API via Script::withAttributes() and Script::attributes(), which will be used in ScriptHandler via those new filters. This way we could also get rid of the DeferScriptOutputFilter and AsyncScriptOutputFilter and solve the problem with adding various attributes. :) Actually i never liked the OutputFilters and there "single use case". Basically every new Attribute would need an own filter and they can cause collisions. Right now we have the possibility to add "defer"- and "async"-attributes at the same time to a Script. :(

[1] https://make.wordpress.org/core/2021/02/23/introducing-script-attributes-related-functions-in-wordpress-5-7/
[2] https://core.trac.wordpress.org/ticket/12009
[3] https://core.trac.wordpress.org/