Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflict with WordPress Block Components Script #263

Closed
mikejolley opened this issue Jul 24, 2019 · 8 comments

Comments

@mikejolley
Copy link

commented Jul 24, 2019

Hi 馃憢

Whilst testing woocommerce/woocommerce-gutenberg-products-block#762 I found that it was a WordPress script (which has dependencies) being broken when AO is used.

To repro, enable:

  • Optimize JavaScript Code?
  • Aggregate JS-files?
  • Also aggregate inline JS?

Then add this somewhere so the script is enqueued:

add_action( 'wp_enqueue_scripts', function() {
   wp_enqueue_script( 'wp-components' );
} );

No other plugins are needed. You'll see these errors on the frontend:

local wordpress test 鈥 Just another WordPress site 2019-07-24 09-45-04

This is what our users see - we use block components on the frontend for JS powerted gutenberg blocks.

Thanks!

@mikejolley

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

Looking further it seems to be specifically the "Also aggregate inline JS?" setting, and the errors are related to missing lodash. Could these lines in wp core be the reason? https://github.com/WordPress/WordPress/blob/3a57fee8918942867927fedff65178a80a59095b/wp-includes/script-loader.php#L131****

@mikejolley

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

Adding an exclusion rule for lodash resolves. This likely needs handling in your plugin since it's not really feasible for plugins to work around this, and it's a conflict with WP core.

Thanks

@futtta

This comment has been minimized.

Copy link
Owner

commented Jul 24, 2019

Morning Mike :-)

  • "also aggregate inline JS" is off by default (for good reasons ;-) )
  • if lodash, then adding window.lodash to AO's comma-separated JS optimization exclusion list should help
  • if that indeed helps I can point you in the direction of how to do so through AO's API

frank

@mikejolley

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

Hi @futtta

Exclusion rules do work, however, I don't feel adding this via the AO API in our plugin is the correct way forward as it could impact other plugins using wp-components.

The conflict is with WordPress itself; perhaps this exclusion should be added as a default just as you have exclusions for the dist directory.

"also aggregate inline JS" is off by default (for good reasons ;-) )

Looking at the support threads I imagine some users just tick all the boxes and think they are "optimising" :)

@futtta

This comment has been minimized.

Copy link
Owner

commented Jul 24, 2019

@mikejolley

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

@futtta That's fine, but tbh I'd expect AO to support the WordPress core scripts - again, we're not talking about scripts in the woo-blocks plugin here, wp-components is part of WordPress core. The aggregate inline feature in AO breaks this.

but can't do so when people change those defaults Mike

If a core script is guaranteed to fail, which wp-components is because of it's lodash dependency, why not force the exclusion and protect users from themselves?

add_action( 'wp_enqueue_scripts', function() {
   wp_enqueue_script( 'wp-components' );
} );

This is all you need to test with to see the issue. No other plugins required.

When this change is rolled into Woo core this problem is going to surface more, and there are no workarounds we can make our side unless we target AO's API specifically. We've had 3 reports for a feature plugin with a small user base so far. Support are simply going to have to instruct those affected users to disable AO or add exclusions manually which is far from ideal.

Go ahead and close this if you don't feel it's worth patching. Maybe as more plugins start implementing blocks and hit this issue you can take another look :)

@futtta

This comment has been minimized.

Copy link
Owner

commented Jul 24, 2019

@mikejolley

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

Ok, there is an ongoing thread about what to do about these dependencies so I think it will be changed at some point, hopefully to Vanilla JS. In the meantime this conflict exists so just be aware if it comes up in your forums; adding an exclusion rule for lodash works around the issue.

Thanks

@mikejolley mikejolley closed this Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.