Skip to content

Conversation

@seanmarthur
Copy link
Contributor

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 dougmoscrop self-requested a review January 8, 2020 20:10
@dougmoscrop dougmoscrop self-assigned this Jan 8, 2020
@dougmoscrop
Copy link
Owner

cool! can you add some tests?

@dryror
Copy link
Contributor

dryror commented Jan 27, 2020

@seanmarthur I've added an example test using your code here dryror@a168712.

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

@dou

Merge tests for OptionalDependencies
@seanmarthur
Copy link
Contributor Author

seanmarthur commented Jan 27, 2020

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
Copy link
Contributor

dryror commented Feb 17, 2020

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

@dougmoscrop
Copy link
Owner

dougmoscrop commented Feb 17, 2020 via email

@dryror
Copy link
Contributor

dryror commented Feb 21, 2020

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
Copy link
Contributor

dryror commented Mar 3, 2020

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
Copy link
Owner

dougmoscrop commented Mar 4, 2020

no need to apologize I appreciate the reminder*

@dougmoscrop dougmoscrop merged commit 181c0a1 into dougmoscrop:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants