Automattic / gutenberg-ramp

Control conditions under which Gutenberg loads - either from your theme code or from a UI
https://wordpress.org/plugins/gutenberg-ramp/
76 stars 15 forks source link

Don't use option if using `wpcom_vip_load_gutenberg ` #39

Closed mikeselander closed 6 years ago

mikeselander commented 6 years ago

The disparity between having an option set and using a code to set said option leads to a good bit of confusion.

For example, when testing the new wpcom_vip_load_gutenberg for which option made the most sense on a client's site, I accidentally saw a false positive in testing 'load' => true after 'load' => 1. I changed the option to true, reloaded my page, saw Gutenberg loaded, and since this is all driven via code, expected that this would work. Came back the next day and spent 20 minutes trying to figure out what I'd done to kill Gutenberg again 🤦‍♂️ .

As a developer, if there is a function to drive functionality, I would expect a change to it to take effect immediately and not have some extra layer that makes me re-load the page several times to se if my change really, really, really, actually took this time. This is extra true in circumstances like VIP Go where the plugin is loaded without the UI being used at all - there's no reason for this option and it just adds potential issues an unnecessary overhead.

mattoperry commented 6 years ago

Hi @mikeselander. Thanks for this. I think your central point here is that it requires a request to reset the option once it's changed, and since the option is stored relatively late, there is always that one page load where the old rather the new criteria are used. This is the subject of this issue, which we decided to not fix for now, but probably should be addressed at some point to avoid this kind of confusion.

As a developer, if there is a function to drive functionality, I would expect a change to it to take effect immediately and not have some extra layer that makes me re-load the page several times to se if my change really, really, really, actually took this time

I definitely agree with you here. @justnorris thoughts? Due to the load order involved here, there aren't a whole lot of great options for how to handle this, but probably the right thing to do would be to carefully force a reload at admin_init if the option has changed.

pyronaur commented 6 years ago

I agree that it's not ideal, but because the loading decision has to happen on plugins_loaded - at the moment there is no way (that I'm aware of) to make this load instantly from theme functions.php file in one load.

Auto-force-reload seems like the only way that we can work around this.

mikeselander commented 6 years ago

I think the best case scenario for this would be for the option not to exist if using the function - the option is completely unnecessary overhead, and if you're using the function the documentation states that the UI is blocked out anyway.

Worst case, could you delete the option and then force a re-load only once if the function is being run?

I'm just spit-balling ideas here, so might be a terrible one 😬

mattoperry commented 6 years ago

@mikeselander no problem -- I get where you're coming from. I've thought a lot about the option. It's unfortunately a necessary evil, at least for now.

One of the design principles of Ramp was that the helper function should be callable in any theme or plugin code -- the rationale being that in particular it should work if called in a theme. Two of the plugin's constituents (sites hosted on WordPress.com and the average generic public user of the plugin, who might control theme code but not much else) made this decision necessary.

The load decision point for Gutenberg is plugins_loaded, which is before theme code executes. So we needed a way to persist the loading criteria to before plugins_loaded even in the function call happened later. Hence, the option, which provides this persistence. You'll notice we also cleanup the option under certain circumstances.

The UI was actually an afterthought -- or at the very least something we tacked on to the core functionality of the plugin. If we'd been able to get away with not using the option, we certainly would have done that, and the UI would probably work differently than it does now.

I hope this clarifies a bit why the option exists.

If there's something we have simply missed in our thinking here (it's totally possible) I'd love to hear your thoughts -- in particular if there's a way you can think of to solve the above requirement without an option? Otherwise we'll probably resolve this particular issue.

mattoperry commented 6 years ago

I think we'll go ahead and resolve this now. Thx for the question.