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.

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

feat(cli): add migrator for cap3 to cap4 #5762

Merged
merged 31 commits into from
Jul 27, 2022
Merged

Conversation

IT-MikeS
Copy link
Contributor

@IT-MikeS IT-MikeS commented Jul 12, 2022

This ports in the Ionic vscode extensions migration functions.

closes #3302

@IT-MikeS IT-MikeS marked this pull request as ready for review July 12, 2022 17:14
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

When running the migrate command it looks like the command hanged, we have ways of showing progress by showing a "spinning wheel", check update command as example when it says "Updating iOS native dependencies with pod install".

Not sure if would be helpful to have "migrate ios" and "migrate android" options too, but would definitely help maintaining the code by having separate files for iOS and Android changes.

If no ios or no android folders are present it will show a ton of errors, the command should check the folder existence first and if it's not there just warn that no migration will be done for that platform and not try to do the migration to that platform to avoid all those errors.

Some variables are out of date, I've commented on the lines, but since the template is shipped with the CLI, not sure if we could get latest versions from the variables.gradle file on the template instead of hardcoding them in code?
Or if it's hard to get from the template since it's zipped, maybe get it from https://github.com/ionic-team/capacitor/blob/main/android-template/variables.gradle

In Capacitor 3 we removed all the code that ran npm install to avoid problems for people using different package managers, but this new migrate command will run npm install.
I think it should at least prompt about it to users so they can skip the install step if desired so they can run their package manager equivalent command.

The changes on the .gitignore files are not helpful as the files already exist.

The migrate command is also doing some Capacitor 2 -> 3 changes, not sure if that was expected or if it was a mistake, if it's supposed to update from 2 -> it should do more things.

Related to that, while this change was from 2 to 3, it was not mandatory and now the init method is gone in capacitor 4, so that should be removed if present
https://capacitorjs.com/docs/updating/3-0#switch-to-automatic-android-plugin-loading

The migrator uninstall @capacitor/storage and install @capacitor/preferences, not sure if it should also search the user code and change the import and usage?

cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

If I choose not to install the packages are not updated in package.json, can the version "text" be updated to "next" so users have just to run the install command, i.e npm install.
This works for npm, not sure about other package managers.

The gitignore changes are still being done but as the files exist it has no effect, better remove that code, it's not really a crucial change to edit the user's gitignore file and to properly work they will need to remove the cache and commit the removal of the files, so I think it's not worth the effort.

If the users chooses to not replace jcenter, it should still add mavenCentral in a few places where it doesn't exist.

There are a lot of join('ios', 'XXX',), where XXX are different files or folders, we have constants for most of them, even for the ios and android folders, in fact it can be configured to not be ios nor android, it should use those constants instead of the hardcoded values. (i.e platformDirAbs`)

cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

platformDirAbs was just an example of one of the values, but most of the join calls should be replaced with all the already existing folder/file paths in https://github.com/ionic-team/capacitor/blob/main/cli/src/config.ts so in case we change that in the future we don't have to go file by file updating the join calls to use the new paths

@IT-MikeS
Copy link
Contributor Author

Oh I see, my bad. Fixing

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

In Capacitor 4 we have removed the deprecated init method, some Capacitor 3 apps might be still using it, so should be removed from MainActivity.java.
Should look something like this but probably with multiple add(Plugin.class); entries depending on the number of plugins installed.

    // Initializes the Bridge
    this.init(savedInstanceState, new ArrayList<Class<? extends Plugin>>() {{
      // Additional plugins you've installed go here
      // Ex: add(TotallyAwesomePlugin.class);
    }});

The migrator should also enable the Android 12 Splash Screen API (see ionic-team/capacitor-docs#17)

cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
cli/src/tasks/migrate.ts Outdated Show resolved Hide resolved
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.

Upgrade Tool
3 participants