morrowdigital / watermelondb-expo-plugin

125 stars 30 forks source link

fix prebuild duplicating strings #42

Closed euharrison closed 6 months ago

euharrison commented 6 months ago

If I run expo prebuild more than once, the injected config strings are executed multiple times, resulting into buggy files

This PR add conditionals to avoid creating duplicated config strings

Please, note that I didn't compiled the plugin then the code style format could be different if you run locally. Feel free to update the PR to match the correct code style guide

Screenshot 2024-04-12 at 02 14 55

Screenshot 2024-04-12 at 02 15 03

Screenshot 2024-04-12 at 02 15 21

Screenshot 2024-04-12 at 02 15 27

euharrison commented 6 months ago

The diff is easier to read removing the whitespaces: https://github.com/morrowdigital/watermelondb-expo-plugin/pull/42/files?diff=split&w=1

brunokiafuka commented 6 months ago

Hi @euharrison, thanks for working on these diffs. Ideally, if you are using expo workflow, you may want to include your native folders into the .gitignore file, as you don't need it to be checked out. Whenever you run eas build, expo will still run the pre-build step. It is also important to mention that when we run the pre-build locally, we delete the native folder to have fresh native builds, which will cause the checks on these diffs always to be true.

# Ignore native files.
ios/*
android/*

Yet, we can still consider adding these safeguards to prevent future issues, I'll let @killerchip decide if we can check in these changes or not.

@euharrison, you can patch the lib for now on your repo while we wait decide if this should be added or not given your use case might be an exception

euharrison commented 6 months ago

Thanks for the quick answer @brunokiafuka !

Sorry, I didn't explained before, I'm using a bare workflow, that's why I'm committing the ios and android folder and I can't run the the expo prebuild --clean because I'll lose changes that is not yet on the plugin config workflow

I hope I can update this workflow in the future, but this is not the reality right now as we have a huge code source

brunokiafuka commented 6 months ago

Thanks for the quick answer @brunokiafuka !

Sorry, I didn't explained before, I'm using a bare workflow, that's why I'm committing the ios and android folder and I can't run the the expo prebuild --clean because I'll lose changes that is not yet on the plugin config workflow

I hope I can update this workflow in the future, but this is not the reality right now as we have a huge code source

@euharrison, because this is specific to your codebase, as an interim solution, I suggest you add a local patch for the plugin while we agree if we should or shouldn't add these checks.

killerchip commented 6 months ago

@euharrison I have no problem to review the changes, and test them. Putting those checks can't do harm (hope these are not famous-last-words :-D ).

But I was wondering in the first place, why you run 'prebuild` since you have are in bare-workflow and you commit your changes. I understand that prebuild is to be run only on 'clean' android directory.

Anyway, another solution, since you don't 'clean' your android directory is to remove the plugin altogether from app.json, since you keep the same files.

But I'll review first chance, anyway

euharrison commented 6 months ago

Hi @killerchip ! Thanks for your answer

I'm working on a project that is running for years without expo, but now that we have the expo config plugin, I'm migrating package by package to the expo config plugin so I can effectively use only the prebuild workflow

But this migration cannot be all together as we have a lot of different packages, and it'll demand a lot of code change at once, which means a bigger possibility to introduce bugs

Then, my current solution is to run expo prebuild freely together with the bare workflow and I can do a "continuous migration" and guarantee that everything is running as expected and all native code is controlled by git. This should take a few months while all packages are migrated and I can finally delete the ios and android folder removing all custom code

Currently, watermelondb is one of this packages, I have it on the bare workflow working fine, but I would like to migrate it now, although it's not the last package that needs to be migrated

But no worries if it's a time consumption for you folks, I can use the fork or simply delay it until it'll be the last package to be migrated :)

wodin commented 6 months ago

I understand that prebuild is to be run only on 'clean' android directory.

My understanding is that this is not true. Expo's own plugins check if they have already been run and avoid running again in that case. e.g. expo-camera's plugin appends the following to build.gradle after checking that the header doesn't exist already:

// @generated begin expo-camera-import - expo prebuild (DO NOT MODIFY) sync-f244f4f3d8bf7229102e8f992b525b8602c74770
def expoCameraMavenPath = new File(["node", "--print", "require.resolve('expo-camera/package.json')"].execute(null, rootDir).text.trim(), "../android/maven")
allprojects { repositories { maven { url(expoCameraMavenPath) } } }
// @generated end expo-camera-import

@euharrison, this may or may not be helpful during your migration process: https://twitter.com/kudochien/status/1749667295335727525

N95JPL commented 6 months ago

Came here to see if other people experienced this. I was also going to write a pull request for fixing it. Nearly all other Plugin modules do a sanity check before running to ensure they do not duplicate the entries. Cheers for the pull-request, I will be using this until if (when?) this gets merged

killerchip commented 6 months ago

Hey Guys Will do a code review and test first chance. I also agree on sanity check. Does not do any harm :-)

On Mon, Apr 15, 2024 at 8:20 PM N95JPL @.***> wrote:

Came here to see if other people experienced this. I was also going to right a pull request for fixing it. Nearly all other Plugin modules do a sanity check before running to ensure they do not duplicate the entries. Cheers for the pull-request, I will be using this until if (when?) this gets merged

— Reply to this email directly, view it on GitHub https://github.com/morrowdigital/watermelondb-expo-plugin/pull/42#issuecomment-2057437308, or unsubscribe https://github.com/notifications/unsubscribe-auth/AELMPHUY2WKLN3VJAUB3RO3Y5QD37AVCNFSM6AAAAABGDPR63CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJXGQZTOMZQHA . You are receiving this because you were mentioned.Message ID: @.***>

killerchip commented 6 months ago

I will close this, as it was merged via ( I had to add a fix on podfile fix) https://github.com/morrowdigital/watermelondb-expo-plugin/pull/43

killerchip commented 6 months ago

@euharrison your fixes are now published in @beta version. Thank you very much for your contribution.

euharrison commented 6 months ago

Thank you very much 🙌