Opened 8 months ago
Last modified 2 months ago
#57643 new enhancement
Remove deprecated @wordpress/nux and replace with empty script and style
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.4 | 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)
Change History (32)
This ticket was mentioned in PR #3775 on WordPress/wordpress-develop by @youknowriad.
8 months ago
#1
This ticket was mentioned in PR #3910 on WordPress/wordpress-develop by @peterwilsoncc.
8 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:
8 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
@
8 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:
- Remove
wp-nux
completely from everywhere, including scritp-loader. Scripts that still depend onwp-nux
will not be loaded, so the plugins that add them will not work. Aswp-nux
has been deprecated and throwing errors for quite some time, it's very likely that these plugins haven't been working for years.- 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.- Leave the JS for
wp-nux
in place, remove the CSS. This is pretty similar to number two aswp-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?
#5
@
8 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
.
#6
@
8 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
#8
@
8 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
@
7 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
@
7 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
@
7 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:
↓ 14
@
7 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
@
7 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
@
7 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:
7 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.
@peterwilsoncc commented on PR #3910:
6 months ago
#17
This ticket was mentioned in PR #4702 on WordPress/wordpress-develop by @Bernhard Reiter.
3 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:
3 months ago
#19
cc/ @ramonjd
@youknowriad commented on PR #4702:
3 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:
3 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:
3 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:
3 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' ); 10 10 * Constants 11 11 */ 12 12 const WORDPRESS_PACKAGES_PREFIX = '@wordpress/'; 13 const WORDPRESS_PACKAGES_DEPRECATED = [ 14 '@wordpress/nux', 15 ]; 13 16 const { getArgFromCLI } = require( `../../node_modules/@wordpress/scripts/utils` ); 14 17 const distTag = getArgFromCLI( '--dist-tag' ) || 'latest'; 15 18 … … function getWordPressPackages( { dependencies = {} } ) { 167 170 * @return {boolean} Is it a @wodpress package? 168 171 */ 169 172 function isWordPressPackage( packageName ) { 173 // Ignore @wordpress packages no longer receiving updates. 174 if ( WORDPRESS_PACKAGES_DEPRECATED.includes( packageName ) ) { 175 return false; 176 } 170 177 return packageName.startsWith( WORDPRESS_PACKAGES_PREFIX ); 171 178 }
#24
@
3 months ago
- Type changed from enhancement to task (blessed)
Converting to a task as this can ship after beta 1 is released.
#25
@
3 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.
#27
follow-up:
↓ 28
@
3 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
@
3 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
@
3 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
@
3 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.
@Bernhard Reiter commented on PR #4702:
2 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 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