-
Notifications
You must be signed in to change notification settings - Fork 927
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
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
end If 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_modules
being somewhere in thepath to podspec file and relied on it to create relative path, which
broke monorepos (since
config
lists 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