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

application-package: refine configuration typings #9503

Closed
wants to merge 1 commit into from

Conversation

paul-marechal
Copy link
Member

Typings now support both a partial and resolved version for our
configurations. A resolved configuration has undefined values
replaced by available defaults, a partial configuration does not.

Related: #9490

How to test

Using FrontendApplicationConfigProvider.set({ ... }) should not force you to define the following fields: applicationName, defaultTheme and defaultIconTheme.

Review checklist

Reminder for reviewers

Sorry, something went wrong.

@paul-marechal paul-marechal added enhancement issues that are enhancements to current functionality - nice to haves core issues related to the core of the application labels May 20, 2021
@paul-marechal paul-marechal force-pushed the mp/application-props branch from 595a55a to 07a6375 Compare May 20, 2021 17:07
@paul-marechal paul-marechal force-pushed the mp/application-props branch from 07a6375 to f3a411b Compare May 20, 2021 17:08
@paul-marechal paul-marechal force-pushed the mp/application-props branch from f3a411b to 956e15a Compare May 20, 2021 17:22
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

There are currently build errors:

lerna ERR! /home/runner/work/theia/theia/dev-packages/application-package/lib/application-props.js:46
lerna ERR!             config: BackendApplicationConfig.DEFAULT,
lerna ERR!                                              ^
lerna ERR! 
lerna ERR! TypeError: Cannot read property 'DEFAULT' of undefined

Typings now support both a `partial` and `resolved` version for our
configurations. A `resolved` configuration has `undefined` values
replaced by available defaults, a `partial` configuration does not.

Co-Authored-By: Nathanael Demacon <nathanael.dmc@outlook.fr>
@paul-marechal paul-marechal force-pushed the mp/application-props branch from 956e15a to 1b1764c Compare May 20, 2021 18:21
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
I confirmed that partial configurations work correctly (ex: when performing a .set).

@paul-marechal
Copy link
Member Author

Closing in favor of #9568.

The new PR is half-way between this one and what @quantumsheep did.

@paul-marechal paul-marechal deleted the mp/application-props branch June 8, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the core of the application enhancement issues that are enhancements to current functionality - nice to haves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants