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.

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.

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.

@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!

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) ?

Fixed in e110119.

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.

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.

@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 to false and Travis check succeeded - 👎

I am not sure why:

1. My npm generated the wrong package-lock.json
2. Travis happily accepted that

I am using Mac FWIW

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: fsevents@1.2.9 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.9: 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.

@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?

@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.

@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?

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 🤷