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

Ensure that insertUsage is always present in preferences #14706

Merged
merged 1 commit into from
Apr 1, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 29, 2019

Description

Follow-up for #14691. Another fix for #14580 :)

I did some more testing with master branch and discover another case when an older version of Gutenberg explodes.

Testing instructions:

  1. (Prerequisite) Update to WordPress v5.1.1
    • In other words, be running the latest version, or at least not WordPress v5.2.0-beta.1
  2. Clear localStorage
    • localStorage.clear() in your Developer Tools Console
  3. Navigate to Posts > Add New
    • Toggle Publish sidebar through Options modal.
  4. Deactivate Gutenberg from the Plugins screen
  5. Navigate to Posts > Add New
  6. Click on the Inserter
    • It uses insertUsage preference from core/editor.
  7. Activate Gutenberg plugin from the Plugins screen
    • Make sure the plugin is a built version of this branch.
  8. Navigate to Posts > Add New
    • You don't need to take any action, just visit. The preference migration takes place automatically.
  9. Deactivate Gutenberg from the Plugins screen
  10. Navigate to Posts > Add New
  11. Click the Inserter
  12. Verify that no error occurs

@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Mar 29, 2019
@gziolo gziolo added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
@gziolo gziolo self-assigned this Mar 29, 2019
@gziolo gziolo requested a review from aduth March 29, 2019 15:35
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving, but at the same time I don't think we need or want this:

  1. Don't need: I can't reproduce the testing instructions causing an error in the master branch, and I wouldn't think it should, since the migration doesn't ever remove the preference from the core/editor store. If the point is about it affecting earlier versions, changing it here doesn't matter since it won't help those earlier versions.
  2. Don't want: We should be skeptical toward anytime we'd ask "How long do we want to keep this dead code?" and the answer would be "I dunno, I guess forever?", which is how I feel toward this. Otherwise, it becomes a burden for future maintainers (and perhaps even a source of future bugs).

As it impacts Monday's Gutenberg 5.4 RC, it can be safe to merge, and we can continue the discussion afterward, since if we later decide it won't hurt to not have the preference, we could similarly safely remove it in the future.

@gziolo
Copy link
Member Author

gziolo commented Mar 29, 2019

I think that it might be also necessary to toggle on/off Publish panel between steps 3 and 4 to trigger saving editor preferences in the local storage. I will try again on Monday with screencast recording enabled, I will merge this change only if I’m able to reproduce on master.

@gziolo
Copy link
Member Author

gziolo commented Apr 1, 2019

I reproduced it on master:

insert-usage

@gziolo gziolo merged commit f5f77d5 into master Apr 1, 2019
@gziolo gziolo deleted the update/insert-usage-default branch April 1, 2019 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants