-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Configure the navigation editor with correct __experimentalFetchLinkSuggestions #21873
Conversation
I am not sure about whether or not should I add |
Size Change: +61 B (0%) Total Size: 817 kB
ℹ️ View Unchanged
|
packages/block-editor/package.json
Outdated
@@ -24,6 +24,7 @@ | |||
"dependencies": { | |||
"@babel/runtime": "^7.9.2", | |||
"@wordpress/a11y": "file:../a11y", | |||
"@wordpress/api-fetch": "file:../api-fetch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the block-editor is a generic JS package not dependent of any API or backend, it shouldn't have a dependency to api-fetch or WordPress in any way. I'm not sure I know a lot about the current issue being fixed here but I though I'd mention this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense, thank you! I moved it to @wordpress/editor
for now, but now @wordpress/edit-navigation
depends on editor just for this one function. I wonder what would be the best place to import this from in both editor AND edit-navigation. I've been thinking about considering the two as disjoint use cases and just copying&pasting that function, but the ultimate expectation is to have the suggestion list work the same in both the editor and in nav-menus.php so I'd rather stick to imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look through the packages and yeah, I'm not sure there is a good place. And my feedback here will probably leave you with more questions than answers.
The main problem with importing from editor
is that I think the unused store for the editor package will be initialized as a side-effect of importing from the package:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/store/index.js#L34
I don't think it's a massive issue, but that might be an argument towards duplicating.
I did wonder if core-data
could be an option (it's already a dependency of edit-navigation
via other packages), though core-data
is very much built around state management as its central purpose, so this might entail changing it to a selector/resolver and storing the suggestions in the reduxy store.
Not sure whether there's a reason that it isn't implemented in that way to begin with (perhaps related to the caching behavior in core-data
, or could just be an artifact of the chronology things were developed in).
All-in-all, I would probably duplicate the function and leave helpful comments above both functions pointing to one another and maybe consider adding unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@talldan I just updated this PR to use a copy of that function. Travis fails because of some package-lock stuff that I will fix tomorrow - your review is highly appreciated!
17c991f
to
1a2c1d2
Compare
…om:WordPress/gutenberg into fix/link-control-does-not-show-suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think this is the lowest friction way to fix, and it doesn't introduce anything that can't be easily tidied up later.
"resolved": "https://registry.npmjs.org/nopt/-/nopt-4.0.1.tgz", | ||
"resolved": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, these changes aren't expected and cause local changes when running npm install
on the master
branch. No fault intended, I just want to better understand how this might have happened, since it was my hope these issues would have been resolved with the changes introduced in #21306.
Could you tell me which version of NPM you're running locally? You can check by npm -v
. Can you elaborate more on what lockfile issues you were remarking about in #21873 (comment) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e110119.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about how Travis is giving a ✅ for these changes as well. Incorrect package lock changes have slipped in a few times now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's the same as what I'd been debugging at #16157 (comment) , I think the issues with Travis are a combination of:
- These are optional dependencies, and specifically in the Linux environment that Travis runs in, they are skipped.
- NPM doesn't (didn't?) do a great job of verifying and updating lockfile for optional dependencies which are skipped, so it never produces the "local changes" we depend on for the Build Artifacts job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth thank you for quickly spotting and fixing the issue! It's pretty interesting - if you take a look at the commit history on this PR, you'll see I've had some trouble with getting the package-lock.json check to work. What finally did the trick was a fresh clone of the repo, ensuring npm 6.14.4
, and doing npm install
on this fresh clone. As a result, we ended up with this "resolved": false
line.
See these two commits specifically:
- 1a2c1d2 - changed
resolved
to""
and Travis check failed - 👍 - a7d3436 - changed
resolved
tofalse
and Travis check succeeded - 👎
I am not sure why:
- My npm generated the wrong
package-lock.json
- Travis happily accepted that
I am using Mac FWIW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, strange indeed. I still think the reason why Travis is passing, based on the prior comment at #16157 (comment), is that node-pre-gyp
is not installed in Linux. At least, it's a transitive dependency of fsevents
, where fsevents
is explicitly skipped:
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
https://travis-ci.com/github/WordPress/gutenberg/jobs/325042262
I would have hoped that running the latest NPM would be enough for these sorts of issues to be avoided. I have noticed a few other cases lately where there seems to be a difference if running npm install
with an existing node_modules
vs. a fresh clone (e.g. #21994 (comment)). It sounds similar to what you describe. I'm not sure how best we could address this, other than assuming it's a bug in NPM and hoping for an upstream fix.
I had contemplated at one point if it might be a good idea to run this specific Travis job in a macOS environment, since it is an option:
https://docs.travis-ci.com/user/reference/osx/
I'd be worried though that it could still leave some variability of accuracy across other OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth if all these cases are about "resolved": false
in package-lock.json, maybe testing for that would be a good workaround? As in cat package-lock.json | grep "resolved": false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel That's an interesting idea, for sure. The issue almost always manifests itself this way [1]. There is some prior art here with processing git diff
to account for some variance [2]. We could do something similar to check for these sorts of values in package-lock.json
.
[1] In the past there's also been some inconsistencies in toggling between http
and https
, though I've seen less of those lately.
[2] Actually not sure if this specific script is even needed anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth if all these cases are about
"resolved": false
in package-lock.json, maybe testing for that would be a good workaround? As incat package-lock.json | grep "resolved": false
?
Proposed at #22237.
I forgot you had suggested a simple approach using grep
. It can work, though the approach in #22237 actually validates against the expected structure of a package-lock.json
. Maybe overkill, but the work's done already 🤷
Description
edit-navigation
package configured the block editor without__experimentalFetchLinkSuggestions
setting. This broke fetching suggestions when creating a new menu item. This PR ensures the setting is present.Fixes #21620
As a side note - should this even be a setting? Any reason not to use the import directly in
LinkControl
?How has this been tested?
Tested locally
Screenshots
Types of changes
Non-breaking change
Checklist: