node: Correct the LocalSource check for npm provider

[Ian2020]

Fix for #377. The check was comparing a string against a PosixPath which always fails. The intention is to see if the package_json file has any parent called "node_modules". Here's an example of what I mean:

@>>> from pathlib import Path
@>>> a = Path("./node_modules/ansi-gray")
@>>> "node_modules" in a.parents
False # WRONG
@>>> Path("node_modules") in a.parents
True # RIGHT

I wanted to write a test for this situation but was unsure how to structure it. I don't think the current tests in test_providers.py ever do a node install so there is no node_modules dir and thus this scenario is never tested. However I think it's more than likely to occur in reality.

[refi64]

I wanted to write a test for this situation but was unsure how to structure it.

We actually do have an existing test for installing local packages: see tests/data/local. However, the package-lock.v3.json inside it uses file::

{
  "name": "@flatpak-node-generator-tests/local",
  "version": "1.0.0",
  "lockfileVersion": 3,
  "requires": true,
  "packages": {
    "": {
      "name": "@flatpak-node-generator-tests/local",
      "version": "1.0.0",
      "dependencies": {
        "subdir": "file:subdir"
      }
    },
    "node_modules/subdir": {
      "name": "@flatpak-node-generator-tests/subdir",
      "version": "1.0.0",
      "resolved": "file:subdir"
    }
  }
}

so it ends up just following the file: branch later on, and thus this particular part wasn't tested. I...don't actually know what scenario results in this branch being taken, could you share the package-lock.json that reproduced the bug? (cc @gasinvein you might remember more about this than I do)

[refi64]

OOF that's a nasty bug! If the intent is to see if any component is node_modules though, this also doesn't work:

>>> Path('node_modules') in Path('node_modules/x').parents
True
>>> Path('node_modules') in Path('a/node_modules/x').parents
False
>>>

Perhaps this should instead be:

Suggested change

	Path('node_modules') not in package_json_path.parents
	'node_modules' not in package_json_path.parts[:-1]

(not actually sure if the :-1 is needed to trim off the last part; the OG code used parents, but I dunno how important excluding the final component is)

[Ian2020]

Good catch - yes I'm not exactly sure if we're just trying to catch any dir called node_modules in the path or if it has to be the last/first. Maybe I'll see if there's a spec for this or something in the node docs.

[gasinvein]

Yeah this definitely should've been package_json_path.parts. My bad.

[Ian2020]

That works for me, I've made the change.

[Ian2020]

My package-lock.json is nothing special I don't think (package-lock.json) - it's just that for many of my packages the second clause of the LocalSource test is already true (package_json_path.exists()):

            if (
                Path('node_modules') not in package_json_path.parents
                and package_json_path.exists()
            ):

...so thanks to the first clause already being defective most of my packages end up considered local.

[Ian2020]

We actually do have an existing test for installing local packages...however, the package-lock.v3.json inside it uses file...so it ends up just following the file: branch later on, and thus this particular part wasn't tested. I...don't actually know what scenario results in this branch being taken

I've rethought my approach here - please take a look at the latest changes. I've got to a solution that's a bit simpler now and I think the existing tests cover all the scenarios adequately.

Your comment above was correct to question why this "LocalSource" branch is needed if the file: case is already handled in another branch. If I remove the LocalSource branch completely we get some test failures but only in tests that use package-lock.v3.json. Those failures (it turns out) are due to the fact that the code tries to process the "root" package as a dependency as well as all the "real" dependencies. To explain further here's part of a typical v3 package-lock:

...
  "packages": {
    "": {
      "name": "@flatpak-node-generator-tests/name-as-dep",
      "version": "1.0.0",
      "dependencies": {
        "word-wrap": "^1.2.3"
      }
    },
    "node_modules/word-wrap": {
      "version": "1.2.3",
      "resolved": "https://registry.npmjs.org/word-wrap/-/word-wrap-1.2.3.tgz",
      "integrity": "sha512-Hz/mrNwitNRh/HUAtM/VT/5VH+ygD6DV7mYKZAtHOrbs8U7lvPS6xf7EJKMF0uW1KJCl0H701g3ZGus+muE5vQ==",
      "license": "MIT",
      "engines": {
        "node": ">=0.10.0"
      }
    }
  }
...

That first package with an empty key is added by npm for the "root" project:

The root project is typically listed with a key of "", and all other packages are listed with their relative paths from the root project folder.

From package-lock.json.

This root project entry causes problems for _process_packages_v2 as it mistakenly processes it as if it were a real dependency. Previously the LocalSource branch was catching this dependency, misclassifying the root project as a LocalSource and thus it was ignored, by not by intention.

If instead we add code to ignore the root package explicitly all the tests pass and we can remove the LocalSource branch completely.

I think this is a better approach and more clearly deals with the issue.

[refi64]

Ah, nice investigation! A few small notes:

• It's not entirely clear to me why the test was reverted?
• It would make the history a bit cleaner if the commits were squashed into two, e.g.:

  node: Correct the LocalSource check for npm
  node: Explicitly ignore the root project
  

  with the first containing the Cleaner expression commit as well. (Normally I'd just do a squash-on-merge or similar, but in this case there's two separate changes, so squashing on merge would collapse them into 1 which isn't quite what we want either.)

[Ian2020]

Thanks!

It would make the history a bit cleaner...

I've squashed it down to the two you suggested, makes sense as it had got a bit messy. I've also fixed the poe blue format issue.

It's not entirely clear to me why the test was reverted?

I regretted adding the test initially but I've restored it now. It felt a bit weird to copy in a whole module (word-wrap) into the project just for a test but it does catch the problem effectively. If there's a better way to test this let me know.

[dbrgn]

Thanks @Ian2020 for the work on this, I'm also running into the problems in #377.

I applied the patch in this PR, but noticed that actual local dependencies (starting with file:) from a npm lockfile v3 cannot be handled anymore:

NotImplementedError: Don't know how to handle package libs/eslint-myplugin in ../../package-lock.json

When I only apply the first commit ("Correct the LocalSource check for npm"), but not the second one, it seems to work. (I run into #382, but that's another issue.)

[Ian2020]

Hey @dbrgn thanks for the feedback, I'll take a look when I get some time.