Skip to content

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

Merged
merged 5 commits into from
Jul 18, 2019
Merged

fix: remove reliance on node_modules in paths to podspecs #550

merged 5 commits into from
Jul 18, 2019

Conversation

simka
Copy link
Contributor

@simka simka commented Jul 17, 2019

Summary:

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

Test Plan:

Tested changes manually to verify correct paths in Podfile.lock, adjusted existing tests

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
@thymikee thymikee requested review from orta and alloy July 17, 2019 12:36
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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path = folder.partition(project_root).last()
path = folder.split(project_root).last

I believe that partition is an activesupport extension, whereas split is core.

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Member

@alloy alloy left a 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 👌

@thymikee thymikee merged commit 4758e4b into react-native-community:master Jul 18, 2019
@alloy
Copy link
Member

alloy commented Jul 18, 2019

Nice one @maciejsimka!

@Salakar
Copy link
Member

Salakar commented Aug 5, 2019

This change breaks local packages in mono-repos by the way;

image

Note the .. at the start of the path even though the path is already fully expanded (local symlinked package by lerna)

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

@thymikee
Copy link
Member

thymikee commented Aug 6, 2019

@maciejsimka do what do you think?

@simka
Copy link
Contributor Author

simka commented Aug 6, 2019

it'd work, but wouldn't the second branch put an absolute path in Podfile.lock? I mean the relative_path is not a relative path at all in case of monorepo.

I came up with possible solution using pathname class, but I hadn't had the time to test it extensively, I'll try to prepare a WIP PR in the evening

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.

autoLinking doesn't work with monorepo
4 participants