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: migrate to new application config #1633

Merged
merged 11 commits into from
Jul 16, 2020

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Jul 15, 2020

This PR is based off #1626 and migrates the appkit codebase to use the new application config.

@changeset-bot
Copy link

changeset-bot bot commented Jul 15, 2020

🦋 Changeset is good to go

Latest commit: 16be1ab

We got this.

This PR includes changesets to release 8 packages
Name Type
merchant-center-application-template-starter Minor
@commercetools-frontend/application-config Minor
@commercetools-frontend/application-shell Minor
@commercetools-frontend/mc-html-template Minor
@commercetools-frontend/mc-http-server Minor
@commercetools-frontend/mc-scripts Minor
playground Minor
@commercetools-local/visual-testing-app Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jul 15, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/9e1pcgf3n
✅ Preview: https://merchant-center-application-kit-git-nm-migrate-app-config.commercetools.vercel.app

@@ -1,2 +1 @@
export { default as processConfig } from './process-config';
export { default as validateConfig } from './validate-config';
Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary to expose it

Comment on lines +34 to +36
// For backwards compatibility, we still support the `env.json` and `headers.json` files.
// TODO: remove in `v17`.
const loadDeprecatedConfig = (
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the part where we load the env.json and headers.json for backwards compatibility

Comment on lines +48 to +58
if (!loadedAppConfig && deprecatedOptions) {
console.warn(
`No custom application config found, attempting to load the config from env.json and headers.json.`
);
const loadedDeprecatedAppConfig = loadDeprecatedConfig(deprecatedOptions);
if (!loadedDeprecatedAppConfig) {
throw new Error(`No configuration for the Custom Application found.`);
}
// Return the legacy config as-is, no need to transform it.
return loadedDeprecatedAppConfig;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the logic where we try to load the deprecated files, in case the new config file was not found.

If you have other suggestions for implementing this logic, please let me know.

<Text.Headline as="h3">Please create a project!</Text.Headline>
<Text.Body>
You are using the Merchant Center in development mode - it is not served
by a proxy (`env.json`). Moreover, you do not have any projects yet. As
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this message was referring to the env.json, I took the liberty to improve the UI a bit, as well as to adjust the information message.

Note that this message only appears in development, when the user has no projects.


#### `loadEnv(configPath: String): Object`
Copy link
Member Author

Choose a reason for hiding this comment

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

The loadEnv export has been removed. I don't think we need to consider it a breaking change, as it's like an internal API.

More information about required values and references can be found in the [Runtime Configuration
](https://docs.commercetools.com/custom-applications/deployment/runtime-configuration) documentation of Custom Applications.

#### `loadHeaders(env: Object, { headersPath: String, cspPath?: String }): Object`
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here, the loadHeaders export has been removed. I don't think we need to consider it a breaking change, as it's like an internal API.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is becomes we can re-add it silently if somebody asks for it.

](https://docs.commercetools.com/custom-applications/deployment/runtime-configuration) documentation of Custom Applications.

#### `loadHeaders(env: Object, { headersPath: String, cspPath?: String }): Object`
#### `processHeaders(applicationConfig: Object, { env: Object, headers: Object }): Object`
Copy link
Member Author

Choose a reason for hiding this comment

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

The processHeaders is the new export, which is basically the same as the loadHeaders, only with different arguments.

@vercel vercel bot temporarily deployed to Preview July 15, 2020 10:29 Inactive
@vercel vercel bot temporarily deployed to Preview July 15, 2020 12:37 Inactive
@vercel vercel bot temporarily deployed to Preview July 15, 2020 12:38 Inactive
@emmenko emmenko marked this pull request as ready for review July 15, 2020 12:45
Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

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

Not much to add. I read it all but I think this is pretty good! :)

More information about required values and references can be found in the [Runtime Configuration
](https://docs.commercetools.com/custom-applications/deployment/runtime-configuration) documentation of Custom Applications.

#### `loadHeaders(env: Object, { headersPath: String, cspPath?: String }): Object`
Copy link
Contributor

Choose a reason for hiding this comment

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

It is becomes we can re-add it silently if somebody asks for it.

@vercel vercel bot temporarily deployed to Preview July 15, 2020 14:41 Inactive
@vercel vercel bot temporarily deployed to Preview July 15, 2020 16:25 Inactive
Copy link
Contributor

@mohib0306 mohib0306 left a comment

Choose a reason for hiding this comment

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

Thanks. Just left a few small comments

packages/application-config/src/load-deprecated-config.ts Outdated Show resolved Hide resolved
cspPath?: string;
};

// List the required fields of `env.json`
Copy link
Contributor

Choose a reason for hiding this comment

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

env.json should be created by the users by themselves and no longer part of the repo. Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure what you mean here but in general we want to start using the new config file (as we do now). I also need to update the documentation and write a release note (separate PR).
People can still use the env.json file, as long as it's supported. For example, in the next major version v17 we can drop support for that.

Does this answer your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. That answers the question

packages/application-config/test/process-config.spec.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview July 16, 2020 07:28 Inactive
@emmenko emmenko merged commit 6e41e67 into nm-app-config-json-schema Jul 16, 2020
@emmenko emmenko deleted the nm-migrate-app-config branch July 16, 2020 08:11
emmenko added a commit that referenced this pull request Jul 16, 2020
* feat: add new package for managing application config

* refactor(config): improve JSON schema

* refactor: further refine JSON schema

* refactor(config): add more text examples

* refactor(config): reimplement schema, add basic utility functions

* refactor(config): remove navbarMenu from schema, implement main build-config function

* refactor(config): use TS, improve schema

* chore(config): command typo

* test(config): for processing config

* chore: ignore build folder

* fix(config): ignore prettier for generated schema.ts

* refactor(config): rename types

* refactor(config): simplify env var interpolation using JSON.parse

* fix: version script to support build and dist folders

* test(config): improve tests setup

* test(config): more validation tests

* refactor(config): attempt to read revision

* chore(config): keep the package private

* docs: improve wording

Co-authored-by: Tobias Deekens <tobias.deekens@commercetools.com>

* refactor(config): rename var

* refactor(config): rename config file to custom-application-config

* test(config): use inline snapshots for better readability

* fix(config): validation order of cloud identifier

* refactor(config): do not use async/await

* refactor: migrate to new application config (#1633)

* refactor: use new application config package to process env and headers

* chore: include application config package in changeset config

* refactor(config): fall back to load deprecated config

* refactor: migrate codebase to use new application config

* refactor: migrate leftovers

* docs: document migration to the new config

* test: fix import

* test(config): for deprecated config files

* fix(vrt): build app for dev mode

* fix(ci): remove unnecessary step

* refactor(config): address feedback

Co-authored-by: Tobias Deekens <tobias.deekens@commercetools.com>
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.

3 participants