[RNMobile] Use tarball instead of git tag for react-native-editor forked dependencies #34886
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ceyhun
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +6.15 kB (+1%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Merged
2 tasks
ceyhun
changed the title
ceyhun
commented
Sep 24, 2021
packages/react-native-bridge/android/react-native-bridge/build.gradle
Outdated
Show resolved
Hide resolved
7 tasks
fluiddot
reviewed
Sep 24, 2021
packages/react-native-bridge/android/react-native-bridge/build.gradle
Outdated
Show resolved
Hide resolved
7 tasks
|
Heads up that I approved all tarball PRs from the forked dependencies, so we could start merging them into |
@ceyhun do you have any concerns regarding starting the merge? Thanks 🙇 ! |
|
All forked dependency PRs have been merged and their URLs in |
fluiddot
reviewed
Sep 27, 2021
| "react-native-keyboard-aware-scroll-view": "git+https://github.com/wordpress-mobile/react-native-keyboard-aware-scroll-view.git#v0.8.8-wp", | ||
| "react-native-linear-gradient": "git+https://github.com/wordpress-mobile/react-native-linear-gradient.git#v2.5.6-wp-1", | ||
| "react-native-hsv-color-picker": "https://raw.githubusercontent.com/wordpress-mobile/react-native-hsv-color-picker/03c0038419326fb86120560cdc3ce958d52cee37/react-native-hsv-color-picker-1.0.1-wp-1.tgz", | ||
| "react-native-keyboard-aware-scroll-view": "https://raw.githubusercontent.com/wordpress-mobile/react-native-keyboard-aware-scroll-view/v0.8.8-wp-1/react-native-keyboard-aware-scroll-view-0.8.8-wp-1.tgz", |
There was a problem hiding this comment.
This is the only dependency that uses tag instead of commit hash in the URL. In the future, when we introduce changes in a forked repository, we would need to generate a tarball and a tag so it will be expected that tarball URLs have the tag, as we have for react-native-keyboard-aware-scroll-view package.
I'm not sure why we didn't generate a new tag for the rest of the repositories but probably it was related to the fact that we're not introducing changes and only the tarball file.
There was a problem hiding this comment.
@fluiddot from working on #36328, it would appear the following is true. I figured I would capture it for posterity.
npmusage within this repository is able to install the third-party packages via git commit hash, so it does not require a tag itself.- The native bridge Android integration extracts the tag name from the tarball file name.
- The native bridge Android integration fetches the forked third-party package using the tag name, which means the relevant tag must be pushed to the forked repository origin.
I.e. npm uses the git reference in the URL. Native bridge Android integration uses the tag name found in the tarball filename.
All that said, it makes sense to me to use the tag name in the URLs found in this file rather than a git commit hash, even though it is not explicitly required by npm. Hopefully the consistency of using a tag name will avoid future confusion.
Thank you for calling out this subject. It helped me greatly.
7 tasks
|
Now that this PR has been merged, we should revert the changes introduced in #35111. @hypest I'd appreciate it if you could take a look at the PR I've just created for this, thanks 🙇 . |
7 tasks
7 tasks
|
Thanks everyone for your hard work here! |
ntsekouras
pushed a commit
that referenced
this pull request
Sep 29, 2021
…ked dependencies (#34886) * Use tarball instead of git tag for rn-editor fork dependencies * Update @react-native-community/slider tarball URL * Trigger CI * Hardcode version numbers in rn-bridge build.gradle * Add readHashedVersion function * Update and rename readHashedVersion * Remove println from extractPackageVersion * Update react-native-editor forked repos references * Fix react-native-keyboard-aware-scroll-view dep url * Update package-lock.json Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
|
Great work folks! Thanks! |
7 tasks
|
Thank you for the changes introduced. Do you have any follow-up work planned to align dependencies of forked dependencies so they install correctly with npm v7? |
Not yet, so far I have only tried to install the dependencies using the tarball solution and the changes from #33892 but I encountered several errors not related to the forked dependencies. I'll try this week again but starting the NPM 7 migration process from scratch, just in case that approach works. Another thing we need to verify is if all forked dependencies support NPM 7, I don't foresee any issue here but it would be worth it to double-check it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3982
Used tarball instead of git tag for react-native-editor forked dependencies.
They are created by running
npm installandnpm packin the forked repository and committing the.tgzfile.These
.tgzfiles are later referenced inpackages/react-native-editor/package.jsoninstead of their git tags.Related to #33424
Forked dependency PRs (already merged ✅ ):
How has this been tested?
Screenshots
N/A
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).