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.

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

Fix external modules version for transitive dependencies #541

Merged

Conversation

salomvary
Copy link
Contributor

This pull request is the same change as #507 plus tests added. I also published a repo with a minimal reproduction case: https://github.com/salomvary/serverless-webpack-bug/tree/master/comp-a (run yarn && node_modules/.bin/serverless package).

What did you implement:

Same as in #507.

How did you implement it:

Same as in #507.

How can we verify it:

Same as in #507.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@salomvary salomvary force-pushed the fix-external-modules-version branch from 2e22f56 to 6c6d299 Compare September 30, 2019 11:46
@salomvary salomvary changed the title Fix external modules version Fix external modules version for transitive dependencies Sep 30, 2019
@salomvary
Copy link
Contributor Author

Sadly, this only solves problem of the missing versions in the generated package.json and eliminates "WARNING: Could not determine version of module XXX" but does not fully solve the problem of fully supporting yarn + multiple modules. The next failure I am getting is:

  Error --------------------------------------------------
 
  yarn install --frozen-lockfile --non-interactive failed with code 1
  error Your lockfile needs to be updated, but yarn was run with `--frozen-lockfile`.

The reason for the frozen lockfile problem is the following. The generated .webpack/dependencies/package.json looks like this (using the exampel repo linked above):

{
  "name": "comp-a-service",
  "version": "1.0.0",
  "description": "Packaged externals for comp-a-service",
  "private": true,
  "scripts": {},
  "dependencies": {
    "classnames": "2.2.6"
  }
}

And .webpack/dependencies/yarn.lock contains this (among a long list of irrelevant devDependencies):

classnames@^2.2.6:
  version "2.2.6"
  resolved "https://registry.yarnpkg.com/classnames/-/classnames-2.2.6.tgz#43935bffdd291f326dad0a205309b38d00f650ce"
  integrity sha512-JR/iSQOSt+LQIWwrwEzJ9uk0xfN3mTVYMwt1Ir5mUcSN6pU+V4zQFFaJsclJbPuAUQH+yfWef6tm7l1quW3C8Q==

Now if I change "classnames": "2.2.6" in package.json to "classnames": "^2.2.6" it will make yarn install --frozen-lockfile --non-interactive no longer fail. Kind of makes sense as this exact version specifier is what we see in yarn.lock and also how it was declared in the original package.lock of the dependent module. Not sure how to proceed from here though, my relationship with yarn/npm/webpack/serverless-webpack is not intimate enough to see any obvious solution...

@salomvary
Copy link
Contributor Author

@HyperBrain What do you think? Is it worth merging a partial fix? Do you have any ideas for a proper solution? I'm kind of stuck here and falling back to ugly workarounds.

@salomvary
Copy link
Contributor Author

I investigated a little further where the dependency@^version vs dependency@version difference comes from. The yarn list command invoked results in a shallow tree not including the original version specifier at all:

$ yarn list --depth=1 --production --json
{
  "type": "tree",
  "data": {
    "type": "list",
    "trees": [
      {
        "name": "comp-b@1.0.0",
        "children": [],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "comp-c@1.0.0",
        "children": [],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "classnames@2.2.6",
        "children": [],
        "hint": null,
        "color": null,
        "depth": 0
      }
    ]
  }
}

Whereas not limiting depth gives us the original version further deep at the tree:

$ yarn list  --production --json
{
  "type": "tree",
  "data": {
    "type": "list",
    "trees": [
      {
        "name": "comp-b@1.0.0",
        "children": [
          {
            "name": "comp-c@file:../comp-c",
            "color": "dim",
            "shadow": true
          }
        ],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "comp-c@1.0.0",
        "children": [
          {
            "name": "classnames@^2.2.6",
            "color": "dim",
            "shadow": true
          }
        ],
        "hint": null,
        "color": "bold",
        "depth": 0
      },
      {
        "name": "classnames@2.2.6",
        "children": [],
        "hint": null,
        "color": null,
        "depth": 0
      }
    ]
  }
}

Could the solution be removing the depth limitation? What was the original purpose of depth=1?

@MrLoh
Copy link

MrLoh commented Jun 30, 2020

any chance to move this along, I'm running into the same issue.

@salomvary what is your current workaround?

@salomvary
Copy link
Contributor Author

@MrLoh I'm using a forked version of serverless-webpack with this pull request merged into it.

@j0k3r j0k3r added the bug label Jul 17, 2020
@j0k3r j0k3r added this to the 5.4.0 milestone Jul 17, 2020
@j0k3r
Copy link
Member

j0k3r commented Jul 17, 2020

Could the solution be removing the depth limitation? What was the original purpose of depth=1?

As far as I can understand, it might be to avoid having to retrieve a huge object because of all the subdeps.
Also, I've just ran each command on a real project and got twice thinner JSON object when using depth=1

@roni-frantchi
Copy link

Using this PR plus bundling the Yarn Workspaces packages has resolved the both the WARNING: Could not determine version of module and Your lockfile needs to be updated, but yarn was run with --frozen-lockfile` for us.

So for instance, if all your monorepo packages are scoped with @acme:

{ // webpack.config.js
  externals: [nodeExternals({ allowlist: [/^@acme/] })],
}

Any chance to move this PR along?...

@j0k3r j0k3r force-pushed the fix-external-modules-version branch from 6c6d299 to 4fa7f2f Compare January 29, 2021 14:28
@j0k3r j0k3r requested review from HyperBrain, miguel-a-calles-mba and a team January 29, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants