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

Jump to bottom

Update ERN platform to support RN 0.67.5 #1869

Merged
merged 1 commit into from
Jan 7, 2023
Merged

Update ERN platform to support RN 0.67.5 #1869

merged 1 commit into from
Jan 7, 2023

Conversation

rthic23
Copy link
Contributor

@rthic23 rthic23 commented Dec 30, 2022 •
edited
Loading

@CLAassistant
Copy link

CLAassistant commented Dec 30, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@friederbluemle
Copy link
Member

Wow nice work @Karthiccc23 - Must have been quite tricky to get all of this set up locally for testing 👍

Since this will be the biggest ERN update in a long time, I think it is important to double and triple check this - I'll try to run through a few variations myself in the coming days.

@friederbluemle
Copy link
Member

update mavenPublisher in miniappRunner to query version from manifest - This commit looks like a nice "atomic" piece of work that can be submitted and merged as a separate PR right away, correct?

friederbluemle
friederbluemle reviewed Jan 5, 2023
View reviewed changes
ern-container-gen-android/src/AndroidGenerator.ts Outdated
dependencies.raw.forEach((d) => {
result.push(d);
});
dependencies.transitive.forEach((d) =>
result.push(`implementation ('${d}') { transitive = true }`),
result.push(`api ('${d}') { transitive = true }`),
Copy link
Member

Choose a reason for hiding this comment

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

Mmmh.. While this might fix the problem with missing transitive dependencies, it is probably not the correct solution in terms of dependency structure (since it will be applied to all dependencies) - Let's discuss tomorrow!

@rthic23
Copy link
Contributor Author

rthic23 commented Jan 5, 2023 •
edited
Loading

update mavenPublisher in miniappRunner to query version from manifest - This commit looks like a nice "atomic" piece of work that can be submitted and merged as a separate PR right away, correct?

yes here is a separate PR #1870 which will merged with manifest PR electrode-io/electrode-native-manifest#237 controlling the versions.

@rthic23 rthic23 force-pushed the karthic/upgradeRN-0.67.5 branch 2 times, most recently from 3eeab2d to 9ca11a4 Compare January 7, 2023 01:35
@friederbluemle friederbluemle force-pushed the karthic/upgradeRN-0.67.5 branch 2 times, most recently from de613ce to 61c8f2b Compare January 7, 2023 02:30
@friederbluemle friederbluemle force-pushed the karthic/upgradeRN-0.67.5 branch from 61c8f2b to 3d53b00 Compare January 7, 2023 02:48
friederbluemle
friederbluemle approved these changes Jan 7, 2023
View reviewed changes
Copy link
Member

@friederbluemle friederbluemle left a comment

Choose a reason for hiding this comment

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

👍

@rthic23 rthic23 merged commit 431386e into electrode-io:master Jan 7, 2023
@rthic23 rthic23 deleted the karthic/upgradeRN-0.67.5 branch March 28, 2023 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@friederbluemle friederbluemle friederbluemle approved these changes

@dudeinthemirror dudeinthemirror Awaiting requested review from dudeinthemirror

@leekuo leekuo Awaiting requested review from leekuo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rthic23 @CLAassistant @friederbluemle