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

Remove the deprecated nux script and style and replace them with empty script and style #3775

Closed

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Dec 16, 2022

This is the PR backporting the WordPress/gutenberg#46110 Gutenberg PR.

The nux package has been deprecated 26 months ago and after checking that removing the remaining code in this package is very low impact, we decide to remove the package and replace it with empty script and style to avoid breaking third-party scripts and styles adding these as dependencies.

Trac ticket https://core.trac.wordpress.org/ticket/57643

@youknowriad youknowriad self-assigned this Dec 16, 2022
@youknowriad youknowriad force-pushed the remove/nux-script-style branch 2 times, most recently from a133875 to a2b250d Compare December 16, 2022 13:15
@youknowriad youknowriad force-pushed the remove/nux-script-style branch from a2b250d to 2b056c9 Compare December 16, 2022 13:22
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

This removes numerous developer APIs which are listed as stable.

While some breakage is inevitable, WordPress aims to maintain backward compaitibility so some attempt to maintain it needs to be included.

@youknowriad
Copy link
Contributor Author

This removes numerous developer APIs which are listed as stable

What are these developer APIs?

@peterwilsoncc
Copy link
Contributor

See WordPress/gutenberg#46110 (comment) about it being a breaking change.

@youknowriad
Copy link
Contributor Author

youknowriad commented Dec 20, 2022

Hi @peterwilsoncc I'm well aware of these small breaking changes, and as discussed on the PR, they were already handled as they should to avoid real breaking changes in production sites.

I was wondering if there's anything that was missed in that discussion, otherwise I don't see any reason to block this as theoretically, there's a number of small breaking changes in most commits to trunk and occasionally bigger ones that go through a different process of deprecations... until removals (example: jQuery upgrade which is orders of magnitude more impactful).

I'm happy to explain the reasoning further here if it's unclear.

@peterwilsoncc
Copy link
Contributor

@youknowriad Please refrain from asking me about back-compat breakages when you are aware of them. It's dismissive and a waste of both of our time.

I raised the hard deprecation of wp-nux and dozens of other stable features in WordPress/gutenberg#41265 but received little in the way of response, so as far as I am concerned the issue has not been resolved.

The fundamental issue remains: there is no attempt here to maintain backward compatibility of stable APIs in accordance with the WordPress philosophy to strive to maintain backward compatibility. The comparison to jQuery is flawed in that jQuery had security issues that could not be resolved without breaking back-compat, thus the key word strive in the WordPress backward compatibility statement.

By doing outreach to major plugins, the issue of backward compatibility issues has not been resolved but hidden. It fails to take in to account the majority of sites using custom builds.

@youknowriad
Copy link
Contributor Author

Please refrain from asking me about back-compat breakages when you are aware of them. It's dismissive and a waste of both of our time.

I think you misunderstood my comment, the PR clearly links to the other discussion that was resolved, so I was asking if there were any other breaking changes that were unhandled that I might not be aware of so I can address them.

I raised the hard deprecation of wp-nux and dozens of other stable features in WordPress/gutenberg#41265 but received little in the way of response, so as far as I am concerned the issue has not been resolved.
The fundamental issue remains: there is no attempt here to maintain backward compatibility of stable APIs in accordance with the WordPress philosophy to strive to maintain backward compatibility.

I think I've replied to your concerns several times as well and felt unheard personally. There's no desire from me or anyone else to not follow the WordPress philosophy.

The backward compatibility strategy is there to ensure that we don't break productions sites for users. But security, sustainability, and the user experience is also as important for WordPress so sometimes, as you said, the keyword is "strive" and that's exactly what I'm doing here.

By keeping APIs that are deprecated, and loading them in the sites of WordPress users, we are both making their websites less performant, thus harming the user experience and less sustainable and with the JS heavy block editor, this issue is very big. In fact, with the scale of WordPress and the number of sites using it, you can easily imagine how many Kbits are wasted.

In this particular instance, there's no way to ensure the long term performance and sustainability of JS scripts without removing deprecated code.

So for long now, with JS code, we've been following this strategy of "striving" backward compatibility. We do our maximum to prevent websites from breaking, we warn people way before time, we give them enough time to update their and even after the time expires, we assess whether it's ok to remove the code or not.

In this particular instance, three major Gutenberg and core contributors agreed that it's ok to remove that code.

I hope this helps clarify the reasoning once more.

@peterwilsoncc
Copy link
Contributor

The backward compatibility strategy is there to ensure that we don't break productions sites for users. But security, sustainability, and the user experience is also as important for WordPress so sometimes, as you said, the keyword is "strive" and that's exactly what I'm doing here.

By keeping APIs that are deprecated, and loading them in the sites of WordPress users, we are both making their websites less performant, thus harming the user experience and less sustainable and with the JS heavy block editor, this issue is very big. In fact, with the scale of WordPress and the number of sites using it, you can easily imagine how many Kbits are wasted.

Experimental APIs have been discussed at length and a path forward is in the works. WordPress/gutenberg#41265 is to discuss the hard deprecation of stable APIs as a separate issue.

I have literally been asking for months for this to be discussed and a resolution sort from project leadership. Until then I maintain that WordPress strives to maintain backward compatibility and, importantly, deleting the wp.nux namespace does not meet this goal.

In this particular instance, there's no way to ensure the long term performance and sustainability of JS scripts without removing deprecated code.

I disagree, wp.nux is deprecated in favour of wp.components.Guide.

Adding deprecation shims to wp-nux will only affect developers making use of the wp-nux package. Developers who have migrated to wp-components will not see a performance issue.

@youknowriad
Copy link
Contributor Author

What I'm hear that you would like to assess whether the current approach is good or not. That's fair. It's an approach that was already set and discussed among the core contributors working on this are of the code and its leadership but it's always good to rediscuss such policies assuming good intent.

I disagree, wp.nux is deprecated in favour of wp.components.Guide.

the guide component yes, and this would avoid double loading code. The PR represents a gain in 3kb of code for users of said plugins that forget the dependency and didn't touch it for years (All the plugins that add the wp-nux dependency but two of them). So it does impact a lot of users.

@peterwilsoncc
Copy link
Contributor

By making wp-nux a deprecation shim for wp.components.Guide, it can be removed as a dependency from each of the scripts and styles:

diff --git a/src/wp-includes/script-loader.php b/src/wp-includes/script-loader.php
index 27defd0659..9614117e49 100644
--- a/src/wp-includes/script-loader.php
+++ b/src/wp-includes/script-loader.php
@@ -1644,12 +1644,10 @@ function wp_default_styles( $styles ) {
 			'wp-editor',
 			'wp-edit-blocks',
 			'wp-block-library',
-			'wp-nux',
 		),
 		'editor'               => array(
 			'wp-components',
 			'wp-block-editor',
-			'wp-nux',
 			'wp-reusable-blocks',
 		),
 		'format-library'       => array(),
@@ -1741,7 +1739,6 @@ function wp_default_styles( $styles ) {
 		'wp-format-library',
 		'wp-list-reusable-blocks',
 		'wp-reusable-blocks',
-		'wp-nux',
 		'wp-widgets',
 		// Deprecated CSS.
 		'deprecated-media',

That will provide the performance advantage for the majority of sites but themes and plugins listing wp-nux as a dependency will continue to work.

The CSS can be safely nulled (using the technique in this PR) as it will no longer be needed as the nux JS pacakge will use the guide styles included in the wp-components JS.

I realise this is easier to say than do but that's often the case.

I'm on leave from the EOD until early Jan so will need to continue any discussion once I return.

@youknowriad
Copy link
Contributor Author

youknowriad commented Dec 22, 2022

There is no "Guide" component in the nux package. We had DotTip components but we quickly decided that they were not good enough and not a good approach for nux, so they were completely deprecated without replacement.

@peterwilsoncc
Copy link
Contributor

I know there is no guide in nux, what I am suggesting is that DotTip use it.

isTipVisible would be shimmed via onFinish;nux-dot-tip added as a custom className, etc.

import Guide from @wordpress/components;

const DotTip = () {
   
  return (
    <>
      <Guide ...>
    </>
  )
}

@youknowriad
Copy link
Contributor Author

I don't really see how it's possible to shim DotTip using the Guide component, I think they have different behaviors: one is for pointing elements in the UI and the other one is about intro guides.

@peterwilsoncc
Copy link
Contributor

Something I missed looking at WordPress/gutenberg#46110 earlier is that I don't think listing wp-nux.css as a dependency of edit-post.css and editor.css was correct.

In the original PR, neither of those packages were updated to include additional styles or to remove imports from the nux package.

In src/wp-includes/assets/script-loader-packages.php, nux isn't listed as a dependency either.

I suspect that removing the nux as a css dependecy in the script-loader file will be adequate. Sites using nux.js, should have it listed as a dependency in the script using it, same for nux.css in the stylesheets.

I guess the question is: were either the editor or edit-post packages using styles from the nux package?

@youknowriad
Copy link
Contributor Author

were either the editor or edit-post packages using styles from the nux package?

The answer is no, both these packages stopped using that package a long time ago. So that dependency was not needed.

@peterwilsoncc
Copy link
Contributor

Thanks Riad, In that case let's change this PR to

diff --git a/src/wp-includes/script-loader.php b/src/wp-includes/script-loader.php
index 093d52b284..c729ccc955 100644
--- a/src/wp-includes/script-loader.php
+++ b/src/wp-includes/script-loader.php
@@ -1644,12 +1644,10 @@ function wp_default_styles( $styles ) {
 			'wp-editor',
 			'wp-edit-blocks',
 			'wp-block-library',
-			'wp-nux',
 		),
 		'editor'               => array(
 			'wp-components',
 			'wp-block-editor',
-			'wp-nux',
 			'wp-reusable-blocks',
 		),
 		'format-library'       => array(),

That will remove the 3KB load without breaking nux for anyone using it. The dev-note for this change can then be to notify developers that they must define wp-nux as a dependency.

For users of the @wordpress/scripts package, this will be handled automatically via the externals so fewer developers will be affected than if the package is deleted.

* wp-nux has been deprecated then removed, an empty script is left here
* to prevent breaking plugins that define it as a depdendency.
*/
$scripts->add( 'wp-nux', false );
Copy link
Contributor

@azaozz azaozz Jan 26, 2023

Choose a reason for hiding this comment

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

Hmm, thinking that registering a non-existing script will actually break all scripts that list it as a dependency. Also that will most likely result in a JS "fatal error" preventing all other JS from running.

The better option here would be to not register non-existing scripts. That way any other scripts that depend on this one will not be loaded which means some plugins will stop working but would prevent eventual "breaking" of the whole page.

@azaozz
Copy link
Contributor

azaozz commented Jan 26, 2023

Think this is mostly a question of code design principles: how to deprecate and then remove old/unused code?

On the PHP side WordPress would generally keep deprecated functions "forever". This is because not breaking websites is a very high priority. However if a whole feature is removed, usually only stubs for the old functions are left in order to avoid fatal errors.

This has been the case when deprecating and later removing JS functionality/features. See: https://core.trac.wordpress.org/browser/tags/5.0/src/wp-admin/js/wp-fullscreen-stub.js.

Imho the "ideal way" to do this is:

  1. Deprecate and output warning that deprecated code is being used.
  2. After ample time remove the code and replace public methods/functions with stubs to not introduce fatal errors. At this point plugins that depend on the deprecated code will stop working, but that will not result in wider breakage/fatal errors.
  3. Eventually remove the stubs few years later.

As far as I understand, the discussion here is whether wp-nux is at the first or second deprecation stage. Seems it is at the second as it has been deprecated and unused for some time. However removing the JS parts of it should not result in JS errors in plugins that expect it to be available. That can be achieved by adding stubs, or by removing wp-nux from script-loader (that will result in dequeueing of of all scripts that depend on it). I'm unsure of which approach is better.

In case many plugin authors weren't aware that wp-nux was deprecated (it hasn't been outputting warnings in the console), perhaps that can be added now to WP 6.2, and the stage 2 (stubs, plugins that still use it stop working) can be added in 6.3?

@youknowriad
Copy link
Contributor Author

In case many plugin authors weren't aware that wp-nux was deprecated (it hasn't been outputting warnings in the console)

For what it's worth, wp.nux was triggering console errors for 27 months now. https://github.com/WordPress/gutenberg/blob/c465d705d969e5d556ac24960304e47d1148072f/packages/nux/src/index.js#L9 So it's clearly on the latest phase of deprecation.

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Jan 26, 2023

My big issue is that I don't see the harm in keeping it around once the CSS dependencies are fixed per PR #3910 -- the pros and cons of this approach are:

Pros

  • no risk of breaking themes or plugins
  • performance gain in core as the unused css is no longer enqueued, perf gain is equivalent to this PR.

Cons

  • two files remain in core that are no longer used

Neutral

  • security team needs to resolve any issue that may be discovered, although this will be the case for legacy branches anyway

Retaining unused libraries in Core is has precedent to maintain backward compatibility, per jquery-form.

// Deprecated, not used in core, most functionality is included in jQuery 1.3.
$scripts->add( 'jquery-form', "/wp-includes/js/jquery/jquery.form$suffix.js", array( 'jquery' ), '4.3.0', 1 );

@azaozz
Copy link
Contributor

azaozz commented Jan 27, 2023

wp.nux was triggering console errors for 27 months now ... So it's clearly on the latest phase of deprecation.

I see. Then why still registering wp-nux in script-loader? This is like telling developers:
This script is available, use it as a dependency if you want.
And then not outputting the script so when they try to use it their scripts throw errors?

As far as I see it there are three options here:

  1. Remove wp-nux completely from everywhere, including scritp-loader. Scripts that still depend on wp-nux will not be loaded, so the plugins that add them will not work. As wp-nux has been deprecated and throwing errors for quite some time, it's very likely that these plugins haven't been working for years.
  2. Add stubs for the public methods in wp_nux and keep it in script-loader. Some features in plugins that use it as a dependency will not work (this has been the case for over two years anyway), but other things there (if any) may still work.
  3. Leave the JS for wp-nux in place, remove the CSS. This is pretty similar to number two as wp-nux has been throwing js errors for a long time, i.e. plugins that are using it as a dependency haven't been working properly anyway.

Assuming that wp-nux have been causing errors and not working for over two years, perhaps option 1 seems most desirable? Then monitor bug reports during WP 6.2 beta and RC (that's nearly two months) and if there are many affected plugins and themes switch to option 3?

@azaozz
Copy link
Contributor

azaozz commented Jan 31, 2023

This PR doesn't seem linked to a trac ticket. Perhaps it should be at:
Trac ticket: https://core.trac.wordpress.org/ticket/57471

@hellofromtonya
Copy link
Contributor

Created a Trac ticket https://core.trac.wordpress.org/ticket/57643 to capture this scope of work, ie for (a) future context/history and (b) ensure it's not lost in the package updates ticket.

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.

4 participants