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

refactor(cli): config refactor #3533

Merged
merged 15 commits into from
Sep 22, 2020
Merged

refactor(cli): config refactor #3533

merged 15 commits into from
Sep 22, 2020

Conversation

imhoffd
Copy link
Contributor

@imhoffd imhoffd commented Sep 10, 2020

  • Config class replaced with a function that returns a readonly object
  • deleted unused properties
  • resolveNode now just uses require.resolve with most statements changed to resolve to root of packages using each package's package.json

@imhoffd
Copy link
Contributor Author

imhoffd commented Sep 10, 2020

@ptitjes Would you be able to review/test the changes around resolveNode? It is possible to use require.resolve if we use <pkg>/package.json as the resolving point for the root of a package. It also works for resolving any JS files within the package. My biggest concern is with getFilePath. Do you remember why this is using resolveNode? The function seems to be for joining paths of non-JS files within a plugin's directory. I don't understand why the code within the path.startsWith('node_modules') if-block is necessary.

@imhoffd imhoffd marked this pull request as ready for review September 10, 2020 16:21
Conflicts:
	cli/src/android/add.ts
	cli/src/android/common.ts
	cli/src/android/doctor.ts
	cli/src/android/open.ts
	cli/src/android/update.ts
	cli/src/common.ts
	cli/src/config.ts
	cli/src/cordova.ts
	cli/src/index.ts
	cli/src/ios/add.ts
	cli/src/ios/common.ts
	cli/src/ios/doctor.ts
	cli/src/ios/open.ts
	cli/src/ios/update.ts
	cli/src/plugin.ts
	cli/src/tasks/add.ts
	cli/src/tasks/copy.ts
	cli/src/tasks/create.ts
	cli/src/tasks/doctor.ts
	cli/src/tasks/init.ts
	cli/src/tasks/list.ts
	cli/src/tasks/new-plugin.ts
	cli/src/tasks/open.ts
	cli/src/tasks/serve.ts
	cli/src/tasks/sync.ts
	cli/src/tasks/update.ts
	cli/src/web/copy.ts
	cli/test/util.ts
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.

Added a minor comment.

Also, the way for users to configure the android studio path was windowsAndroidStudioPath and linuxAndroidStudioPath. The PR seems to remove those config options (so breaking change? should be documented?).
In open.ts it's still mentioned that you should use them, so texts should be updated for the new way of configuring them.

And in capacitor.config.json the plugins object appears the first one, since it can grow a lot if users add a lot of configurations, the other values would be hard to find, probably better to print the plugins object the last

cli/src/config.ts Outdated Show resolved Hide resolved
imhoffd and others added 2 commits September 18, 2020 09:40
Co-authored-by: jcesarmobile <jcesarmobile@gmail.com>
@imhoffd
Copy link
Contributor Author

imhoffd commented Sep 18, 2020

I didn't mean to break the Android Studio path stuff at this time, so I fixed it. But I think we should change it before the final release.

Fixed the plugins appearing first in some cases. 👍

@imhoffd imhoffd merged commit c8a0595 into main Sep 22, 2020
@imhoffd imhoffd deleted the config-refactor branch September 22, 2020 16:20
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.

2 participants