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): support TS/JS config files #3756

Merged
merged 22 commits into from
Nov 9, 2020
Merged

feat(cli): support TS/JS config files #3756

merged 22 commits into from
Nov 9, 2020

Conversation

imhoffd
Copy link
Contributor

@imhoffd imhoffd commented Nov 3, 2020

Devs can now have capacitor.config.ts (or capacitor.config.js) files, for example:

import { CapacitorConfig } from '@capacitor/cli';

const config: CapacitorConfig = {
  appId: 'com.capacitorjs.testapp',
  appName: 'capacitor-testapp',
  bundledWebRuntime: false,
  webDir: 'build',
  plugins: {
    SplashScreen: {
      launchShowDuration: 0,
    },
  },
};

export = config;

depends on #3755
resolves #3141

@imhoffd imhoffd added this to the 3.0.0 milestone Nov 3, 2020
@imhoffd imhoffd mentioned this pull request Nov 3, 2020
@aparajita
Copy link

Still debating if TS config files should be supported

What are the technical issues in implementing that? I assume you would compile on the fly.

@imhoffd
Copy link
Contributor Author

imhoffd commented Nov 3, 2020

What are the technical issues in implementing that? I assume you would compile on the fly.

We'd have to ship typescript as a dependency and then do something like what Stencil does.

@aparajita
Copy link

We'd have to ship typescript as a dependency and then do something like what Stencil does.

Yeah, I figured it would be something like that. If you already have working code you can steal from Stencil, you might as well do it. You could add TypeScript as an optional dependency.

@imhoffd imhoffd changed the title feat(cli): support capacitor.config.js config files feat(cli): support TS/JS config files Nov 3, 2020
@imhoffd imhoffd marked this pull request as ready for review November 4, 2020 17:32
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 few comments on the definitions.
ios.scrollEnabled is missing.

Also, since init won't work if a .ts/.js file is present, don't need to change the code in a few places, I commented the places where it doesn't need to change.

cli/src/declarations.ts Show resolved Hide resolved
cli/src/declarations.ts Outdated Show resolved Hide resolved
cli/src/declarations.ts Outdated Show resolved Hide resolved
cli/src/tasks/init.ts Show resolved Hide resolved
cli/src/tasks/init.ts Show resolved Hide resolved
cli/src/cordova.ts Show resolved Hide resolved
@imhoffd
Copy link
Contributor Author

imhoffd commented Nov 9, 2020

Added scrollEnabled (isn't documented currently, btw: https://capacitorjs.com/docs/config)

I reworded the bundle option and clarified the default value for allowsLinkPreview per your suggestions 👍

Also, since init won't work if a .ts/.js file is present, don't need to change the code in a few places, I commented the places where it doesn't need to change.

I chose not to use the config name in init. I think it might be easier when maintaining it in the future (easier to search for capacitor.config.json), but I don't feel strongly about it

Since there is already a common confusion about the android package name and the application ID, I think we should not use package ID here, better use App ID.

For now I changed it back, but we should probably have a bigger discussion because Ionic, Capacitor, and Appflow all call it different things. 😂 I copied this description from create-capacitor-app, but I can change it to App ID there too.

@imhoffd imhoffd merged commit a52775f into main Nov 9, 2020
@imhoffd imhoffd deleted the dynamic-config branch November 9, 2020 21:08
extConfigType: 'ts',
extConfigName,
extConfigFilePath: extConfigFilePath,
extConfig: requireTS(ts, extConfigFilePath) as any,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be quite useful to optionally support exporting a () => Promise<ExternalConfig> instead of just ExternalConfig.

Would you accept a pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking about this as well. I would be happy to look at a PR that adds that functionality.

*
* @since 1.0.0
*/
url?: string;

Choose a reason for hiding this comment

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

Would it be possible to clarify why we say this This is not intended for use in production?
You can see under #5075 this question was asked. In #4122 people still use it and it fixes problems like #1373

Choose a reason for hiding this comment

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

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.

Support dynamic configuration files
6 participants