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

Declare drivers as optionalDependencies (Fix #3059) #3081

Merged
merged 1 commit into from Oct 29, 2019

Conversation

@kapouer
Copy link
Contributor

kapouer commented Mar 3, 2019

Fix #3059

This is a simpler approach using optionalDependencies.
It's already supported by npm, and yarn --pnp is happy with it.

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Mar 3, 2019

So what does yarn --pnp does in this case? pack all the optional drivers to .pnp.js ? I asked those questions in #3059 because I really would like to know what actually happens :)

@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented Mar 3, 2019

I PR'd this to be more explicit about the changes involved. Also that made me actually check what was happening in practice, as you asked.

The problem with optionalDependencies is that npm or yarn (even in --prod mode) try to install all of them and don't fail if they can't.

That's why the "peerDependencies" + "peerDependenciesMeta" was implemented by yarn: it silence the warnings about uninstallable peer dependencies, and do both the job of not installing the modules that are not already installed, and properly setup .pnp.js with the peerDependencies that are actually available.

@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented Mar 3, 2019

Please don't merge this PR, it's work in progress.

@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented Mar 3, 2019

The problem with peerDependenciesMeta is that it's not supported by npm, and there is no sign showing it will be soon. However missing peerDependencies only shows warnings, not failures, and i think most npm users just use them as hints when something goes wrong.

@kapouer kapouer force-pushed the kapouer:yarn-pnp branch from f4bb519 to 42062bd Mar 3, 2019
@larixer

This comment has been minimized.

Copy link

larixer commented May 9, 2019

@kapouer Is this PR still work in progress? And if yes, what are the blockers?

@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented May 9, 2019

It can be merged, however it might make npm users yell about the warnings.

@jthn

This comment has been minimized.

Copy link

jthn commented Oct 29, 2019

Wondering if this could get merged. I'd like to migrate a project to using Yarn PnP, and I'd love to be able to use it without forking this project.

@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented Oct 29, 2019

Update: npm/cli#224 now peerDependenciesMeta is supported by npm !

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 29, 2019

Woohoo!
Not sure that declaring drivers as peerDependencies is correct approach, though, because it contradicts the original intent of peerDependencies: stating dependencies that consumer of the library is supposed to satisfy.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 29, 2019

Wait, peerDependenciesMeta actually solves that. Then it's OK to merge, after driver versions are brought up-to-date

Copy link
Collaborator

kibertoad left a comment

Please bump driver versions

@kapouer kapouer force-pushed the kapouer:yarn-pnp branch from 42062bd to 7a0c47f Oct 29, 2019
@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented Oct 29, 2019

Done. I supposed no new driver has been added since march 2019.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 29, 2019

Thanks!

@kibertoad kibertoad merged commit a5c23a4 into knex:master Oct 29, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 29, 2019

@jthn Released in 0.20.1

@kapouer

This comment has been minimized.

Copy link
Contributor Author

kapouer commented Oct 29, 2019

It's supported but still prints a WARNing.
If that's okay, it'd be safer to keep it that way until npm is fixed: It's a freshly deployed feature there, it might have been rolled back for any other reason.
I've notified upstream (in npm/cli#224) just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.