feat(compat): add cordova support #1288
Conversation
merceyz
force-pushed
the
cordova-compat
branch
2 times, most recently
from
c415954 to
8e22e7d
Compare
|
This PR looks like it would solve a problem that I'm having 🎉 , but it also looks like it could be distributed as an external plugin rather than being incorporated directly into berry. AFAICT, this would be a useful plugin wherever any |
| //#region Ignored | ||
| @Command.Boolean('--save') | ||
| save!: boolean; | ||
|
|
||
| @Command.Boolean('-P,--save-prod,--production') | ||
| production!: boolean; | ||
| //#endregion |
There was a problem hiding this comment.
Why not this (or to type them as ?: boolean, at worst)?
| //#region Ignored | |
| @Command.Boolean('--save') | |
| save!: boolean; | |
| @Command.Boolean('-P,--save-prod,--production') | |
| production!: boolean; | |
| //#endregion | |
| @Command.Boolean('--save') | |
| save: boolean = false; | |
| @Command.Boolean('-P,--save-prod,--production') | |
| production: boolean = false; |
There was a problem hiding this comment.
Yarn has no version of these flags so their value doesn't really matter, however if it was to be set, the default would be true. But they're just ignored, so I marked them as such
| const descriptor = workspace.dependencies.get(ident.identHash)!; | ||
| const resolution = project.storedResolutions.get(descriptor.descriptorHash)!; | ||
| const pkg = project.storedPackages.get(resolution)!; | ||
| console.log(`+ ${pkg.name}@${pkg.version}`); |
There was a problem hiding this comment.
Damn that ain't pretty :( Not sure it should be in the default compat plugin after all ...
There was a problem hiding this comment.
Yeah, it's not pretty. On the bright side, it's only printed when someone runs yarn npm install foo (npm can be replaced with something else to hide it even more, open for suggestions here). Should probably make the command hidden as well.
|
I'm pro putting this in a separate plugin to make this opt-in. I would definitely propose using a separate name for the plugin commands, especially given the existence of |
|
I'll be with @bgotink here and say that the complexity would be a better fit as a standalone contrib plugin (perhaps not even inside our own repository). |
|
Was there ever a separate contrib plugin made for this? I can't find one |
|
No, I ended up making one for internal use at the company I work for since I required more changes than I initially made here. |
What's the problem this PR addresses?
Cordova is currently hardcoded to run npm commands which makes using cordova with yarn a bit problematic. Getting them to detect which package manager to use has proven to not be happening any time soon. See apache/cordova-fetch#46 and apache/cordova-cli#292 (comment)
How did you fix it?
Implement a proxy that translates the required npm commands to yarn commands