-
Notifications
You must be signed in to change notification settings - Fork 932
fix: remove reliance on node_modules in paths to podspecs
#550
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
fix: remove reliance on node_modules in paths to podspecs
#550
Conversation
Previous implementation assumed `node_modules` being somewhere in the path to podspec file and relied on it to create relative path, which broke monorepos (since `config` lists real filepaths instead of, as I assumed previously, symlinks). New implementation subtracts project root path from absolute path to podspec file for the purpose of generating relative path. Resolves #537
| absolute_podspec_path = File.dirname(podspec_path) | ||
| relative_podspec_path = File.join(root, absolute_podspec_path.partition('node_modules').last(2).join()) | ||
| folder = File.dirname(podspec_path) | ||
| path = folder.partition(project_root).last() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| path = folder.partition(project_root).last() | |
| path = folder.split(project_root).last |
I believe that partition is an activesupport extension, whereas split is core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, even if partition is core I think split is more descriptive and straight-forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split().last returns nil when the left and right side is the same, while partition.last() converts this to empty string.
I pushed a fix for that in 500a868
alloy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it that “still have to adjust tests in native_modules.rb” is outdated, because this looks 👌
|
Nice one @maciejsimka! |
|
This change breaks local packages in mono-repos by the way; Note the Changing the pod line to the following worked for me (whether that's the best solution I'm not sure): if relative_path.include?('node_modules')
pod spec.name, :path => File.join(root, relative_path)
else
pod spec.name, :path => relative_path
endIf this makes sense lmk and I'll send a PR. Thanks |
|
@maciejsimka do what do you think? |
|
it'd work, but wouldn't the second branch put an absolute path in Podfile.lock? I mean the I came up with possible solution using |

Summary:
Previous implementation assumed
node_modulesbeing somewhere in thepath to podspec file and relied on it to create relative path, which
broke monorepos (since
configlists real filepaths instead of, asI assumed previously, symlinks). New implementation subtracts project
root path from absolute path to podspec file for the purpose of generating
relative path.
Resolves #537
Test Plan:
Tested changes manually to verify correct paths in
Podfile.lock, adjusted existing tests