dougmoscrop / serverless-plugin-include-dependencies

MIT License
184 stars 40 forks source link

Support optional peer dependencies via `packageJson.peerDependenciesMeta` #44

Closed seanmarthur closed 4 years ago

seanmarthur commented 4 years ago

I am not quite sure how to properly write a test to support this use case, but this code is tested against my serverless project.

A reproduction case would be to add the knex package to a serverless project. The knex package.json looks like this:

  "peerDependencies": {
    "mssql": "^5.1.0",
    "mysql": "^2.17.1",
    "mysql2": "^1.7.0",
    "pg": "^7.12.1",
    "sqlite3": "^4.1.0"
  },
  "peerDependenciesMeta": {
    "mssql": {
      "optional": true
    },
    "mysql": {
      "optional": true
    },
    "mysql2": {
      "optional": true
    },
    "pg": {
      "optional": true
    },
    "sqlite3": {
      "optional": true
    }
  },

knex will only be used with a maximum of 1 of those peer dependencies, and they are specified as optional via the peerDependenciesMeta config. Without the code change in this PR, the build will fail, complaining about one of those dependencies being missing (Error: [serverless-plugin-include-dependencies]: Could not find mssql.)

dougmoscrop commented 4 years ago

cool! can you add some tests?

dryror commented 4 years ago

@seanmarthur I've added an example test using your code here https://github.com/dryror/serverless-plugin-include-dependencies/commit/a1687125998b1227c15089566c2a48089c274c32.

Would love to see this merged in, but didn't want to take over your PR. :)

@dou

seanmarthur commented 4 years ago

I merged the tests written by @dryror into my repo so they are now included in this PR.

Pls review/verify them - I cannot test locally at the moment.

dryror commented 4 years ago

@dougmoscrop any chance of getting this merged in now that there are tests? 🍻

dougmoscrop commented 4 years ago

Yes sorry! Today is a holiday in Canuckistan but I'll get to it tomorrow

On Mon., Feb. 17, 2020, 1:26 p.m. Rory Drysdale, notifications@github.com wrote:

@dougmoscrop https://github.com/dougmoscrop any chance of getting this merged in now that there are tests? 🍻

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dougmoscrop/serverless-plugin-include-dependencies/pull/44?email_source=notifications&email_token=AAEPGFERBKOLSEAUIFN7LW3RDLJGJA5CNFSM4J2FBHI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL7KBMQ#issuecomment-587112626, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEPGFCNS4MBZDLZFSJM62TRDLJGJANCNFSM4J2FBHIQ .

dryror commented 4 years ago

Yes sorry! Today is a holiday in Canuckistan but I'll get to it tomorrow On Mon., Feb. 17, 2020, 1:26 p.m. Rory Drysdale, @.***> wrote: @dougmoscrop https://github.com/dougmoscrop any chance of getting this merged in now that there are tests? 🍻 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44?email_source=notifications&email_token=AAEPGFERBKOLSEAUIFN7LW3RDLJGJA5CNFSM4J2FBHI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL7KBMQ#issuecomment-587112626>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEPGFCNS4MBZDLZFSJM62TRDLJGJANCNFSM4J2FBHIQ .

Great, thanks @dougmoscrop

dryror commented 4 years ago

Sorry to keep bugging you @dougmoscrop, but any chance of getting this merged in? Would love to update my knex dependency, but this is preventing me from being able to. 🍻

dougmoscrop commented 4 years ago

no need to apologize I appreciate the reminder*