Install issue when yarn workspaces are used

andrehtissot / cordova-plugin-fcm-image-support

FCM Image Support for Cordova iOS

MIT License

2 stars 2 forks source link

Install issue when yarn workspaces are used #1

Open coulson84 opened 4 years ago

coulson84 commented 4 years ago

npx cordova plugin add cordova-plugin-fcm-image-support

can't find node_modules/cordova-ios/bin/templates/scripts/cordova/lib/prepare.js when using yarn workspaces because dependencies get hoisted to the top level of the repo so is not necessarily at the location where the installation script expects it to be.

It should be possible to walk up the directories looking for a cordova installation within node_modules but this could break cordova for other packages if it is installed globally and the patch is applied there as it is not found anywhere else in the tree. Not sure if this will cause issues or not as have not looked at the source code yet.

andrehtissot commented 4 years ago

Interesting.

Unfortunately, the internal package.json files present no link to the workspace's root. Which would require, indeed, the walk up.

I don't think it would break anything, as the patch it very minimal, replacing:

origPkg !== pkg for !origPkg.startsWith(pkg); and
origPkg === pkg for origPkg.startsWith(pkg);

Which allows other bundle identifiers as a subdomain, like the one used in this extension: ${appConfig.mainProductBundleIdentifier}.FCMNotificationService.

The only issue would be the uninstall, as it does aim to restore pre-patched state. Uninstalling on one project could break others. So for this "walking up" behavior, the unpatching should be avoided.

@coulson84, what do you think?

coulson84 commented 4 years ago

Could the patch that is applied be useful to other plugins (or at least not cause any other issues with them) because maybe it could be possible to submit a PR to the cordova-ios package so that the patch is already included upstream as standard thereby removing the need to do any patching during (un)install?

andrehtissot commented 4 years ago

Yes, I'll create a PR there.

But this might take a while to get merged and would only be applied to latest ios versions.

I have an idea of deal with an uninstall check. This fix will be implemented soon.