Skip to content
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.

Sign up for GitHub

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

Jump to bottom

missing peerDependencies #117

Closed
happysalada opened this issue Apr 23, 2022 · 6 comments
Closed

missing peerDependencies #117

happysalada opened this issue Apr 23, 2022 · 6 comments
Labels
bug Something isn't working nodejs

Comments

@happysalada
Copy link
Contributor

here is the problem I encountered.
While trying to build this project https://github.com/happysalada/primo

building

> primo-server@1.2.6 build /nix/store/15q09sxbbgbca9l53cjvlg0arv5k88li-primo-server-1.2.6/lib/node_module>
> svelte-kit build

> Cannot find package 'svelte' imported from /nix/store/1h2ri6f620r47vqg54bv3iamyf360z9y-__at__sveltejs__>
Did you mean to import svelte-3.47.0/lib/node_modules/svelte/compiler.js?
Did you mean to import svelte-3.47.0/lib/node_modules/svelte/compiler.js?
    at new NodeError (internal/errors.js:322:7)

the problematic package is

    "node_modules/@sveltejs/vite-plugin-svelte": {
      "version": "1.0.0-next.41",
      "resolved": "https://registry.npmjs.org/@sveltejs/vite-plugin-svelte/-/vite-plugin-svelte-1.0.0-next.41.tgz",
      "integrity": "sha512-2kZ49mpi/YW1PIPvKaJNSSwIFgmw9QUf1+yaNa4U8yJD6AsfSHXAU3goscWbi1jfWnSg2PhvwAf+bvLCdp2F9g==",
      "dependencies": {
        "@rollup/pluginutils": "^4.2.0",
        "debug": "^4.3.4",
        "kleur": "^4.1.4",
        "magic-string": "^0.26.1",
        "svelte-hmr": "^0.14.11"
      },
      "engines": {
        "node": "^14.13.1 || >= 16"
      },
      "peerDependencies": {
        "diff-match-patch": "^1.0.5",
        "svelte": "^3.44.0",
        "vite": "^2.9.0"
      },
      "peerDependenciesMeta": {
        "diff-match-patch": {
          "optional": true
        }
      }
    },

svelte is in the peerDependencies. It looks like at the moment those are not installed in the node_module of the related dependency.
when I check dependencies installed for that package, I only see

lrwxr-xr-x    98 root 31 Dec  1969  sourcemap-codec -> /nix/store/3niwynnjsys1rizrin9nxxr10gbc1sar-sourcemap-codec-1.4.8/lib/node_modules/sourcemap-codec
dr-xr-xr-x     - root 31 Dec  1969  @rollup
lrwxr-xr-x    94 root 31 Dec  1969  estree-walker -> /nix/store/i8vq4vxa5igzlkp1z3w58rra9v3zyn7f-estree-walker-2.0.2/lib/node_modules/estree-walker
lrwxr-xr-x    93 root 31 Dec  1969  magic-string -> /nix/store/9sfjl8ixrd5sh0yks76y4r745l6xhm93-magic-string-0.25.7/lib/node_modules/magic-string
lrwxr-xr-x    89 root 31 Dec  1969  svelte-hmr -> /nix/store/y4vb39bby7rvnrvlsfg3a543vhfq7y5y-svelte-hmr-0.14.9/lib/node_modules/svelte-hmr
lrwxr-xr-x    86 root 31 Dec  1969  picomatch -> /nix/store/iqmv2i7ps8mx3x0yzraywcbm5pw4di6w-picomatch-2.3.1/lib/node_modules/picomatch
lrwxr-xr-x    78 root 31 Dec  1969  kleur -> /nix/store/kda75cwi7rn1ccb5f2ll6368389ar3bq-kleur-4.1.4/lib/node_modules/kleur
lrwxr-xr-x    78 root 31 Dec  1969  debug -> /nix/store/b878bb1lc5js7sxvzpdxhfq9qzlmw154-debug-4.3.3/lib/node_modules/debug
lrwxr-xr-x    72 root 31 Dec  1969  ms -> /nix/store/76wsm74kb9v1bxxnn848jbdpk0y0q5rj-ms-2.1.2/lib/node_modules/ms

most probably we should add peerdependencies as well if possible.
@DavHau
Copy link
Member

DavHau commented Apr 23, 2022

The problem with peerDependencies is that different versions of NPM handle them differently. See https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependencies

In npm versions 3 through 6, peerDependencies were not automatically installed, and would raise a warning if an invalid version of the peer dependency was found in the tree. As of npm v7, peerDependencies are installed by default.

That means depending on the npm version used upstream, peerDependencies might be completely ignored, and therefore not part of the lock file, in which case we should ignore them too, and in other npm versions they are treated as normal dependencies and are part of the lock file, in which case we should install them.

If we want to have a nice UX, we should detect automatically which npm version was used upstream. We could to that by looking at:

  • lock file version (version 2 or higher indicates npm version 7 or higher)
  • engine field of package.json (might have a minimum version for npm)
  • devDependnecies in package.json (might have a npm version specified)

But some projects won't have a lock file and none of the required fields set in package.json, therefore we still need a flag, allowing the user to manually toggle peer deps on/off.

@happysalada
Copy link
Contributor Author

very nice find.
I tried to install the dependency with the lockfile with version 1 and with version 2
the previous snippet was after I updated the lockfile.
Before I updated it, the lockfile looks like

    "@sveltejs/vite-plugin-svelte": {
      "version": "1.0.0-next.37",
      "resolved": "https://registry.npmjs.org/@sveltejs/vite-plugin-svelte/-/vite-plugin-svelte-1.0.0-next.37.tgz",
      "integrity": "sha512-EdSXw2rXeOahNrQfMJVZxa/NxZxW1a0TiBI3s+pVxnxU14hEQtnkLtdbTFhnceu22gJpNPFSIJRcIwRBBDQIeA==",
      "requires": {
        "@rollup/pluginutils": "^4.1.2",
        "debug": "^4.3.3",
        "kleur": "^4.1.4",
        "magic-string": "^0.25.7",
        "svelte-hmr": "^0.14.9"
      },
      "dependencies": {
        "debug": {
          "version": "4.3.3",
          "resolved": "https://registry.npmjs.org/debug/-/debug-4.3.3.tgz",
          "integrity": "sha512-/zxw5+vh1Tfv+4Qn7a5nsbcJKPaSvCDhojn6FEl9vupwK2VCSDtEiEtqr8DFtzYFOdz63LBkxec7DYuc2jon6Q==",
          "requires": {
            "ms": "2.1.2"
          }
        },
        "ms": {
          "version": "2.1.2",
          "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.2.tgz",
          "integrity": "sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w=="
        }
      }
    },

it doesn't even show the dependency, however I get the exact same error.
So in the previous version of npm, the lockfile is just wrong and we have no way to fix this.
This tends to make me think that we shouuldn't worry about npm version and always try to install peerDependencies to be on the safe side (if they are not used, we loose just a bit of space, if they are we prevent mistakes).

If we don't install them, we should have a way to make an override to add a dependency. something like

vite-plugin-svelte = {
   add-missing-deps = {
       buildInputs = old: old ++ [ self.svelte ];
   }
}

(or something like that, I couldn't find a way to manually add a dependency already in the dependency graph).

@DavHau
Copy link
Member

DavHau commented Apr 23, 2022 •
edited
Loading

We can't just try to install peerDependencies by default, because they might not be part of the lock file and then translation will fail.
Though we could install them whenever they are part of the lock file.

You can add dependency edges via the inject parameter. See an example here:
https://github.com/bobanetwork/boba/blob/b6b200d74e6e90c63ade6180adb1c06585c9b77b/flake.nix#L162-L172

@happysalada
Copy link
Contributor Author

very nice secret api!
do they support wildcard ?
at the moment my inject looks like

      inject = {
        "@sveltejs/vite-plugin-svelte"."1.0.0-next.37" = [
           ["svelte" "3.46.4"] ["vite" "2.8.4"]
         ];
        "@tiptap/extension-blockquote"."2.0.0-beta.26" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-bold"."2.0.0-beta.26" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-bubble-menu"."2.0.0-beta.55" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-bullet-list"."2.0.0-beta.26" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-code"."2.0.0-beta.26" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-code-block"."2.0.0-beta.37" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-document"."2.0.0-beta.15" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-dropcursor"."2.0.0-beta.25" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-floating-menu"."2.0.0-beta.50" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-gapcursor"."2.0.0-beta.34" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-hard-break"."2.0.0-beta.30" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-heading"."2.0.0-beta.26" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-highlight"."2.0.0-beta.33" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-history"."2.0.0-beta.21" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-horizontal-rule"."2.0.0-beta.31" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-image"."2.0.0-beta.27" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-italic"."2.0.0-beta.26" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-link"."2.0.0-beta.36" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-list-item"."2.0.0-beta.20" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-ordered-list"."2.0.0-beta.27" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-paragraph"."2.0.0-beta.23" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-strike"."2.0.0-beta.27" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
        "@tiptap/extension-text"."2.0.0-beta.15" = [
           ["@tiptap/core" "2.0.0-beta.174"]
         ];
      };

wondering if I can do something like

      inject = {
        # wildcard to match all versions ?
        "@tiptap/extension-blockquote"."*" = [
           # wildcard to match all versions ? Or perhaps I just don't specify the version ?
           ["@tiptap/core" "*" ]
        # wildcard in the name to match a series of dependencies ?
        "@tiptap/extension-*"."*" = [
           ["@tiptap/core" "*"]
         ];
      };

just wondering about those, no worries if they are not implemented.

@DavHau
Copy link
Member

DavHau commented Apr 24, 2022

That API should be improved and documented. The wildcard idea needs to be added as well.

@DavHau DavHau mentioned this issue Apr 24, 2022
Closed
@DavHau DavHau added the bug Something isn't working label Apr 24, 2022
@DavHau DavHau added the nodejs label Jun 16, 2022
@tgunnoe tgunnoe mentioned this issue Jul 9, 2022
Open
6 tasks
@DavHau
Copy link
Member

DavHau commented Jul 13, 2022

I'm sorry, I just noticed that my previous comments on this issue are highly misleading. We are in fact installing exactly the dependencies from the lock file. Dependency resolution already has happened by NPM which created the lock-file, Therefore we cannot and should not do anything regarding the peerDependencies.

The problems you were running into @happysalada are due to the build toolchain not being able to cope with symlinks. This requires either patching the toolchain, or using installMethod=copy.

With installMethod=copy + some fixes in #194, the package builds fine without having to use any injects

@DavHau DavHau closed this as completed Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working nodejs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@happysalada @DavHau