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.

Sign up for GitHub

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

Add support for scoped npm package linking in local-cli #17000

Closed
wants to merge 1 commit into from
+40 −0
Closed

Add support for scoped npm package linking in local-cli #17000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions local-cli/link/__tests__/link.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,42 @@ describe('link', () => {
done();
});
});

it('should replace forward slashes in scoped package names for android', () => {
const isInstalledAndroid = jest.fn(() => false);
const registerNativeModule = sinon.stub();
const registerDependencyAndroid = jest.fn();
const config = {
getProjectConfig: () => ({ ios: {}, android: {}, assets: [] }),
getDependencyConfig: sinon.stub(),
};

const mockDependencyConfig = [{
config: { ios: {}, android: {}, windows: null, assets: [], commands: {}, params: [], },
name: '@scope/react-native-package',
}];

jest.setMock('../ios/isInstalled.js', sinon.stub().returns(true));

jest.mock('../getDependencyConfig.js', () => () => mockDependencyConfig);

jest.mock(
'../android/isInstalled.js',
() => isInstalledAndroid,
);

jest.setMock(
'../ios/registerNativeModule.js',
registerNativeModule,
);

jest.mock('../android/registerNativeModule.js', () => registerDependencyAndroid);

const link = require('../link').func;

return link(['react-native-test'], config).then(() => {
expect(isInstalledAndroid).toHaveBeenCalledWith({}, '@scope_react-native-package');
expect(registerDependencyAndroid).toHaveBeenCalledWith('@scope_react-native-package', {}, {}, {});
});
});
});
2 changes: 2 additions & 0 deletions local-cli/link/link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ const dedupeAssets = (assets) => uniqBy(assets, asset => path.basename(asset));


const linkDependencyAndroid = (androidProject, dependency) => {
dependency.name = dependency.name.replace(/\//g, '_');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right place for this change. First and foremost thing is that it mutates dependency object making this function no longer pure.

Another thing is that I can't tell where the new value with / replaced to _ should be used. In your comment you said that it breaks include statement in Gradle.

Maybe you should do this replacement directly inside the function that puts include statement into Gradle file?

Copy link
Contributor

Choose a reason for hiding this comment

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

E.g. here https://github.com/vekerdyb/react-native/blob/e9e841628fc77faa9bd271c86a031d0518d5e7e6/local-cli/link/android/patches/makeSettingsPatch.js#L22, make const escapedName = name.replace() and done :) (hopefully)

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, that will only work if this name isn't re-used anywhere else. IIRC, we use it in many places, so we need to have a consistent name across different functions. As a compromise, we can introduce a new function, something like sanitizeName or similar that only transforms a given dependency name into a version that will be understandable by Grade and other parts of the react-native link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, that will only work if this name isn't re-used anywhere else

Yup, and that way it's explicit where we sanitize the name. Doing it globally on a dependency can have side effects as the name field is supposed to be the name of the package.

Function is a great idea that plays well with our design. I think I've seen it already in generator subfolder where they do the same for react-native init (sanitize the name when creating Android main application). We could potentially reuse that as well :)

Copy link
Author

@vekerdyb vekerdyb Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback! Unfortunately I am quite busy at the moment, but will get back to this mid February, unless it is picked up by someone else by then.

I think I've seen it already in generator subfolder where they do the same for react-native init (sanitize the name when creating Android main application). We could potentially reuse that as well

I agree, I faintly remember seeing init working as expected now.

Copy link
Contributor

@rozele rozele Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I published #18207 to fix the issue for Android and iOS.

Copy link
Collaborator

@Titozzz Titozzz Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fix for android, but you also fixed iOS at the same time by importing the correct file ? Looks like it @rozele

capture d ecran 2018-03-06 a 09 56 05

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll test your fix and make mine from your branch if this works

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because your branch is being landed, i'll apply my fixes to it 💛

Copy link
Collaborator

Choose a reason for hiding this comment

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

There you go: #18275


if (!androidProject || !dependency.config.android) {
return null;
}
Expand Down