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

use stdlib-provided method to locate plugin dir #138

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

jesse-savary
Copy link
Contributor

@jesse-savary jesse-savary commented Oct 29, 2022

Description

Updates the node_modules locating code to use require.resolve, which should support more than 5 levels of nesting and be more reliable than manually checking for node_modules.

Details

Motivation

Noticed that newer versions of onesignal-expo-plugin natively supports monorepos, making our local patch redundant. However I believe that our patch uses a better method to locate the directory so I am submitting it now.

Scope

What will change:

  • Monorepos with more than five layers of nesting will be supported.

Testing

Manual testing

Cloned my fork locally, built it, and tested using expo prebuild -p ios with a live app.

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have personally tested this on my device, or explained why that is not possible
  • I have tested this on the latest version of the plugin
  • I have tested this on both Android and iOS, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

@jesse-savary
Copy link
Contributor Author

As it turns out, this plugin does not work on our monorepo without this PR.

@brismithers
Copy link
Contributor

brismithers commented Oct 31, 2022

Thanks for your contribution @jesse-savary , certainly more elegant! I tried to run your changes against this example and get this error:

[ios.xcodeproj]: withIosXcodeprojBaseMod: Cannot find module 'onesignal-expo-plugin/package.json'

Hoping you can help me understand what is different between your project and my example project. Thanks!

@jesse-savary
Copy link
Contributor Author

Thanks for your contribution @jesse-savary , certainly more elegant! I tried to run your changes against this example and get this error:

[ios.xcodeproj]: withIosXcodeprojBaseMod: Cannot find module 'onesignal-expo-plugin/package.json'

Hoping you can help me understand what is different between your project and my example project. Thanks!

I will look into this very soon

@brismithers
Copy link
Contributor

brismithers commented Feb 14, 2023

@jesse-savary I'm curious if you've been able to identify what I'm doing wrong attempting to get your change working. Thanks!

@alexbchr
Copy link

alexbchr commented Mar 1, 2023

I just tested this PR and it actually works fine. I first encountered the same issue as you @brismithers when testing using yarn link to locally link this package to our Expo App:

[ios.xcodeproj]: withIosXcodeprojBaseMod: Cannot find module 'onesignal-expo-plugin/package.json'

However, after looking at the resolve path, I saw that require.resolve was attempting to resolve the file path from the "linked" package itself, and not from our App node_modules folder. So actually this seems to be an issue with yarn link. If I installed the onesignal-expo-plugin straight from npm and then replaced the build/withOneSignalIos.js file with the one generated by this PR, the problem goes away and I am now able to run expo prebuild in my project without encountering issue #159

Any plans to merge and release this soon (even on a beta or canary channel)?

@brismithers
Copy link
Contributor

@alexbchr thanks for that pointer! Using that approach everything checks out.

@brismithers brismithers merged commit b539ce5 into OneSignal:main Mar 3, 2023
@brismithers
Copy link
Contributor

Thanks @jesse-savary !

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.

None yet

3 participants