Make WordPress Core

Opened 15 months ago

Closed 7 months ago

#57643 closed enhancement (wontfix)

Remove deprecated @wordpress/nux and replace with empty script and style

Reported by: youknowriad's profile youknowriad Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.0
Component: Editor Keywords: has-patch gutenberg-merge needs-dev-note 2nd-opinion close add-to-field-guide
Focuses: Cc:

Description

Remove the deprecated nux script and style and replace them with empty script and style.

The nux package has been deprecated 26 months ago. This ticket proposes:

  • Removing the package
  • Replacing it with empty script and style to avoid breaking third-party scripts and styles adding these as dependencies.

Is a very low impact, i.e. after checking that removing the remaining code in this package.

Reference:

Attachments (1)

Screen Shot 2023-02-08 at 12.20.44 pm.png (62.8 KB) - added by peterwilsoncc 15 months ago.

Download all attachments as: .zip

Change History (35)

This ticket was mentioned in PR #3775 on WordPress/wordpress-develop by @youknowriad.


15 months ago
#1

This is the PR backporting the https://github.com/WordPress/gutenberg/pull/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

This ticket was mentioned in PR #3910 on WordPress/wordpress-develop by @peterwilsoncc.


15 months ago
#2

Alternative approach to PR https://github.com/WordPress/wordpress-develop/pull/3775 for deprecating nux https://github.com/WordPress/gutenberg/pull/46110

In the discussion for the original pull request, it seems the wp-edit-post and wp-editor styles no longer require the wp-nux css file as a dependency.

This removes the CSS dependencies while retaining the JavaScript to ensure JavaScript continues to function for extenders still using the nux package. As the styles are not needed, this will see a small performance gain for most WordPress installs.

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

@hellofromTonya commented on PR #3775:


15 months ago
#3

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.

#4 @hellofromTonya
15 months ago

  • Keywords 2nd-opinion added

Marking with 2nd opinion as discussion of how to proceed is happening in the PR 3775.

@azaozz summarized options here (copying since the PR was linked after the comment):

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?

Last edited 15 months ago by hellofromTonya (previous) (diff)

#5 @peterwilsoncc
15 months ago

  • Keywords close added

To log my concerns posted on the original PR.

The argument for hard deprecating JavaScript APIs is that there is a performance benefit from doing so. However, deleting a package that is unused by WordPress Core doesn't provide such a benefit, the file is only enqueued if a theme or plugin developer is using it.

Any performance gain from the original PR comes from a bug fix: removing the unused CSS dependencies for wp-edit-post and wp-editor. Based on the discussion in the original PR, this could have been done a while ago. The same performance gain is achieved by my alternative pull request without breaking backward compatibility.

To address each of the items in the list above:

Item one is likely to break more than just features using wp-nux. Build tools are ubiquitous -- and encouraged via wp-scripts -- so many themes and plugins using wp-nux will find none of their JavaScript is enqueued.

Item two seems like additional work for no gain. WordPress doesn't see any performance improvements. It's contributor time spent breaking a feature that doesn't need to be broken.

Item three is incorrect: the package has been logging warnings, console.warn, for some time but the script does not cause JavaScript errors and functions as intended for theme and plugin developers.

There is precedent for retaining scripts no longer used by WordPress Core in the code base to maintain backward compatibility, jquery-forms, suggest, jcrop, and cropper haven't been used for some time but are maintained. There is no performance impact from doing so.

In my view, removing wp-nux is a WordPress developer convenience at the expense of backward compatibility for theme an plugin developers. Nothing is lost from retaining it.

I think this ticket can be closed as wontfix.

Last edited 15 months ago by peterwilsoncc (previous) (diff)

#6 @youknowriad
15 months ago

I still think the best option is to actually remove the package and to follow @azaozz's first option. Yes, it's a developer convenience but as explained, I don't think it's at the expense of the user at all. The package has been deprecated a long time ago, no one is really using it and it's probably broken already as it is.

I also wanted to note that we'll be having an issue every time we'll want to perform a major @wordpress packages update on Core. See thread here https://wordpress.slack.com/archives/C02QB2JS7/p1675248959410629 so the minimum we should be doing is removing the package from the package.json dependencies.

In that sense, I'm willing to compromise here if it allows this to move forward and implement the second option proposed by @azaozz

#7 @flixos90
15 months ago

@youknowriad +1 on going with @azaozz's 2nd option.

#8 @peterwilsoncc
15 months ago

Based on my testing with a mini-plugin, it seems to be functioning fine per the screen shot above. I certainly wouldn't consider it broken.

The positioning can be a little screwy but it pretty much works as I'd expect if I were developing a plugin. A similar test with a custom sidebar worked as expected too, the tip didn't show up until I activated the custom sidebar.

I can only see cons with option two:

  • it breaks sites
  • it takes contributors away from bug fixes during the beta period in order to do so

I don't think it's possible to move the package to wordpress-develop. The @wordpress/* npm packages are in scope for the hacker1 program so a way of fixing any issues that come up will need to be retained. At present, it's not possible to publish from wordpress-develop

#9 @hellofromTonya
14 months ago

Based on my testing with a mini-plugin, it seems to be functioning fine per the screen shot above. I certainly wouldn't consider it broken.

The positioning can be a little screwy but it pretty much works as I'd expect if I were developing a plugin. A similar test with a custom sidebar worked as expected too, the tip didn't show up until I activated the custom sidebar.

I can only see cons with option two:

  • it breaks sites
  • it takes contributors away from bug fixes during the beta period in order to do so

@flixos90 @youknowriad @azaozz What do you think about @peterwilsoncc's observations with option 2?

#10 @flixos90
14 months ago

I would like to better understand why removing this package is a priority. I certainly understand the rationale that it has been deprecated for a very long time, but at least in the past this has not been a justification for removing the code. FWIW deprecated WordPress core code never gets removed - which I personally find questionable, but that's how it is right now.

To be clear, I'm not opposed to removing the package, but I acknowledge @peterwilsoncc's concerns for potential breakage. If there is a strong case why keeping this package is a maintenance burden, maybe the benefits outweight the risks, but I would like to see that context being clarified. At least right now I don't see it explained in this ticket or the PR.

Regardless of what we decide here going forward, I think at this point this ticket should be punted to 6.3. It is the only enhancement still in the milestone that hasn't had a first commit yet (which means it shouldn't be here in the first place), and I think with RC approaching soon it is not a good idea to merge this now. This change in particular would be one that is not testable through Gutenberg, so it would need the WP core beta cycle. It may even benefit from being committed early.

#11 @peterwilsoncc
14 months ago

Thanks Felix:

Just to note that fixing the CSS dependencies per my linked pull request #3910 can probably be considered a bug as neither of the packages the CSS listed as a dependency for use any of the styles. Fixing this may warrant a sentence in the miscellaneous dev note.

I agree modifying/removing the script is an enhancement best committed early if that's the approach that is decided upon. Now the package has been removed from the Gutenberg repo, it's not possible to test any implications for the change upstream.

#12 follow-up: @youknowriad
14 months ago

Removing the package is not a priority and I agree that it's too late in the 6.3 release to commit this removal. In fact, I opened the PR way early in the 6.2 cycle.

Now why removing the package, as I explained it's not just about the package itself, if it was just a single package, that would be fine, but we deprecate hundreds of APIs on each release and it's untenable to keep all these APIs (especially JavaScript) around forever.

I'm happy to remove the package early in the 6.3 cycle.

#13 @hellofromTonya
14 months ago

  • Milestone changed from 6.2 to 6.3

Okay there's consensus that it's too late for 6.2. Moving to 6.3 for continued discussion.

#14 in reply to: ↑ 12 @peterwilsoncc
14 months ago

Replying to youknowriad:

Now why removing the package, as I explained it's not just about the package itself, if it was just a single package, that would be fine, but we deprecate hundreds of APIs on each release and it's untenable to keep all these APIs (especially JavaScript) around forever.

I don't this this answers the question as to the specific advantage of removing this package. It suggests to me there isn't a benefit in the same there would be no benefit in removing the deprecated jQuery Forms and other libraries no longer used by core.

I've created #57827 as a bug fix for removing the unneeded loading of CSS and put it on the performance focus.

@youknowriad commented on PR #3910:


14 months ago
#15

Well obviously, that's the minimum we should be doing. I think it's not enough and we discussed this over and over again. There's no point in repeating the same arguments over and over. We can hear if we don't want to.

#16 @peterwilsoncc
13 months ago

In 55628:

Script Loader: Remove unused wp-nux CSS dependency.

Neither wp-edit-post nor wp-editor use the styles included in the wp-nux CSS file. This removes nux as a dependency from the registration of the two files.

Props flixos90, youknowriad, peterwilsoncc.
Fixes #57827.
See #57643.

This ticket was mentioned in PR #4702 on WordPress/wordpress-develop by @Bernhard Reiter.


10 months ago
#18

Running npm run sync-gutenberg-packages -- --dist-tag=wp-6.3 currently results in the following error:

Running "wp-packages:update" task
Updating WordPress packages (--dist-tag=wp-6.3)
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @wordpress/nux@wp-6.3.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

after which script execution resumed, thus typically hiding the error behind a lot of other output.

However, this results in package.json not being updated with the expected version bumps to @wordpress/* packages.

This PR seeks to remediate the issue by removing the dependency on @wordpress/nux. There seems to be some related discussion on @wordpress/nux deprecation in https://core.trac.wordpress.org/ticket/57643.

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

@Bernhard Reiter commented on PR #4702:


10 months ago
#19

cc/ @ramonjd

@youknowriad commented on PR #4702:


10 months ago
#20

For what it's worth I tried removing this dependency before (See https://github.com/WordPress/wordpress-develop/pull/3775 ) and @peterwilsoncc had disagreed about it because of backward compatibility concerns. So we might want to check that again, main the additional release make this feasible now.

It's also worth clarifying that this is going to create issue for us every time we try to do a package update.

@Bernhard Reiter commented on PR #4702:


10 months ago
#21

Thank you @youknowriad!

I guess one workaround -- to unblock the sync script -- would be to add the wp-6.3 dist tag to the latest existing @wordpress/nux package; but I'm not sure if that's possible for a deprecated npm 🤔

Otherwise, I guess we'll have to remove the dependency before running the sync script, and then add it back afterwards 😕

@youknowriad commented on PR #4702:


10 months ago
#22

Otherwise, I guess we'll have to remove the dependency before running the sync script, and then add it back afterwards

That's what I did the last time to generate the lock. It's a bit painful though.

@peterwilsoncc commented on PR #4702:


10 months ago
#23

Would this work in wp-dev without the need to change wp-scripts?

  • tools/release/sync-gutenberg-packages.js

    diff --git a/tools/release/sync-gutenberg-packages.js b/tools/release/sync-gutenberg-packages.js
    index f5c8fbcdf7..c5c12b140f 100644
    a b const { zip, uniq, identity, groupBy } = require( 'lodash' ); 
    1010 * Constants
    1111 */
    1212const WORDPRESS_PACKAGES_PREFIX = '@wordpress/';
     13const WORDPRESS_PACKAGES_DEPRECATED = [
     14        '@wordpress/nux',
     15];
    1316const { getArgFromCLI } = require( `../../node_modules/@wordpress/scripts/utils` );
    1417const distTag = getArgFromCLI( '--dist-tag' ) || 'latest';
    1518
    function getWordPressPackages( { dependencies = {} } ) { 
    167170 * @return {boolean} Is it a @wodpress package?
    168171 */
    169172function isWordPressPackage( packageName ) {
     173        // Ignore @wordpress packages no longer receiving updates.
     174        if ( WORDPRESS_PACKAGES_DEPRECATED.includes( packageName ) ) {
     175                return false;
     176        }
    170177        return packageName.startsWith( WORDPRESS_PACKAGES_PREFIX );
    171178}

#24 @audrasjb
10 months ago

  • Type changed from enhancement to task (blessed)

Converting to a task as this can ship after beta 1 is released.

#25 @peterwilsoncc
10 months ago

  • Milestone changed from 6.3 to 6.4
  • Type changed from task (blessed) to enhancement

I'm not happy with this been a task. There is talk of a complete rewrite of the package and moving it in to WordPress-Develop, both of which I think require further discussion.

I've got publishing rights on npm but am neither foolish not brave enough to run npm dist-tag add @wordpress/nux@6.0.0 wp-6.3 to allow npm run sync-gutenberg-packages to run without problems. I'm not sure what the effect would be on a deprecated package.

I've moved this to 6.4 for follow up, this will be after the session "Unifying the backward compatibility policies within the WordPress-develop and Gutenberg repos" at the community summit, so I think we'll be in a better place then.

#26 @stevenlinx
10 months ago

  • Keywords add-to-field-guide added

#27 follow-up: @isabel_brison
10 months ago

I'm not 100% sure if npm dist-tag add @wordpress/nux@6.0.0 wp-6.3 will work for a deprecated package, but it should be safe enough to try.

It's not my favourite solution and it would be good to keep investigating how to otherwise handle this, but for now, if it works, it'll remove a painful hurdle in the release process so let's give it a go :)

#28 in reply to: ↑ 27 @peterwilsoncc
10 months ago

Replying to isabel_brison:

I'm not 100% sure if npm dist-tag add @wordpress/nux@6.0.0 wp-6.3 will work for a deprecated package, but it should be safe enough to try.

Thanks Isabel.

Tagging a deprecated package seems to work, although the tag listing is suppressed on the NPM page.

> npm dist-tag add @wordpress/nux@6.0.0 wp-6.3
+wp-6.3: @wordpress/nux@6.0.0

Running npm run sync-gutenberg-packages -- --dist-tag=wp-6.3 in wp-dev discovered the packages that have been recently published and completed error free. It throws the deprecation warning and moves on.

#29 @isabel_brison
10 months ago

I just published new package versions for --dist-tag=wp-63 and running npm run sync-gutenberg-packages -- --dist-tag=wp-6.3 I found that, though it builds without errors, two packages are being updated to the wrong versions:

"@wordpress/components": "23.9.0", (should be "25.1.5")
"@wordpress/data": "8.6.0", (should be "9.5.3")

So adding the dist-tag to @wordpress/nux@6.0.0 unfortunately hasn't solved the whole problem.

Let's look into other approaches to fix this before the next release.

#30 @peterwilsoncc
10 months ago

@isabel_brison I've removed the tag, npm dist-tag rm @wordpress/nux wp-6.3, as an apparent success with incorrect versions of key packages is a much bigger problem than an obvious failure.

I've been hesitant to say this but I think GB#46110 needs to be reverted in trunk and the affected wp/* branches:

  • the failing sync script represents a significant hurdle for the security team doing backports. This increases the chance a mistake will be made during a security release
  • WordPress-Develop (and thus WordPress releases) are still consuming the script, so deleting the package was premature and needs to occur after WordPress-Develop stops consuming it.
Last edited 10 months ago by peterwilsoncc (previous) (diff)

@Bernhard Reiter commented on PR #4702:


9 months ago
#31

Closing. As Riad mentioned, there's https://github.com/WordPress/wordpress-develop/pull/3775; plus the dependency was removed from Gutenberg in https://github.com/WordPress/gutenberg/pull/46110, which is now being discussed to be reverted in https://github.com/WordPress/gutenberg/issues/52444.

This ticket was mentioned in Slack in #core-editor by mike. View the logs.


7 months ago

#33 @hellofromTonya
7 months ago

  • Milestone changed from 6.4 to Future Release

After asking in the Making Core slack #core-editor channel, thinking this ticket is a close candidate.

@gziolo replied:

I think @peterwilsoncc led the efforts to bring it back. I see it’s getting published regularly with v8.4.1 from yesterday that is tagged for wp-6.4 release. I guess we can close the ticket.

Hey @peterwilsoncc can you confirm please?

For now, I'll move it to Future Release to get it out of the milestone ahead of 6.4 Beta 1 release start. But thinking this one is a close candidate.

#34 @peterwilsoncc
7 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

@hellofromTonya That's correct, I reverted the hard deprecation upstream in Gutenberg#52444 as it was was causing other @wordpress/* packages to be downgraded when syncing to WordPress-Develop.

In [55628] for #57827 the CSS dependencies were fixed to prevent wp-nux.css from being enqueued incorrectly with core packages -- this achieves the performance benefits discussed above and on the original pull request.

I agree, this can be closed. If @wordpress/nux is to be nulled out it will need to be done upstream to ensure the syncing script for packages continues to work as expected.

Note: See TracTickets for help on using tickets.