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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vendor extracted js modules not available in editor. #2371

Closed
wants to merge 1 commit into from

Conversation

oxyc
Copy link
Contributor

@oxyc oxyc commented Dec 25, 2019

See #2370

I know sage has always registered the scripts at the same time they're enqueued so this solution might not be great to everyone.

Another option is to somehow fix it in the laravel mix config I guess.

@Log1x
Copy link
Member

Log1x commented Dec 25, 2019

editor.js should not be enqueued on the frontend.

By default, I do not enqueue vendor.js as it is not required with the default behavior, but can simply be enqueued in enqueue_block_editor_assets the same as it is in wp_enqueue_scripts.

This has been taken into consideration and in the future– it would be ideal if all vendor dependencies related to editor.js are bundled with it despite .extract() – but it is not high on the todo list at the moment (and probably won't be considered until webpack 5)

@oxyc
Copy link
Contributor Author

oxyc commented Dec 25, 2019

editor.js should not be enqueued on the frontend.

No of course not, this PR only registers it.

If you prefer duplicating the wp_enqueue_script line rather than separating registrations from enqueues I'm fine changing the PR.

As for leaving it as it is, I'd vote no as you can't write es6 code without having to manually add vendor.js as a dependency. Something which isn't very straightforward to figure out since the "bug" doesn't issue any warnings/notices and comes from a babel runtime not mentioned anywhere in the sage code.

@Log1x
Copy link
Member

Log1x commented Dec 26, 2019

No of course not, this PR only registers it.

Ahh I see now. Sorry, I had initially replied on my phone.

As for leaving it as it is, I'd vote no

I agree with you. We should add it in, even if commented out by default (open to opinions) – at least until we have vendor splitting between the app and editor.

I would like to stick to using the original method of wp_enqueue_script, though.

More or less, just keep the original but change to something along the lines of (untested):

add_action('enqueue_block_editor_assets', function () {
    if ($manifest = asset('scripts/manifest.asset.php')->get()) {
        wp_enqueue_script('sage/vendor.js', asset('scripts/vendor.js')->uri(), $manifest['dependencies'], $manifest['version']);
        wp_enqueue_script('sage/editor.js', asset('scripts/editor.js')->uri(), $manifest['dependencies'], $manifest['version']);

        wp_add_inline_script('app/vendor.js', asset('scripts/manifest.js')->contents(), 'before');
    }

    wp_enqueue_style('sage/editor.css', asset('styles/editor.css')->uri(), false, null);
}, 100);

@oxyc
Copy link
Contributor Author

oxyc commented Dec 31, 2019

@Log1x I updated the PR but can't really see a good way to have it commented out since:

  1. I think we should have it as a dependency of sage/editor.js otherwise I can potentially see some third party plugin breaking things. If you disagree I can of course remove it as a dependency.
  2. As the inlined manifest needs to come before sage/vendor.js we'd have to provide a commented out version of that too.

@Log1x Log1x mentioned this pull request Feb 29, 2020
@Log1x
Copy link
Member

Log1x commented Feb 29, 2020

I'm sorry! I completely forgot about this PR til today. I have added similar changes to #2413 so I'm closing this as it'll get merged it with that.

@Log1x Log1x closed this Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants