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: show warning if user set unknown target and explicitly set it as browser #7578

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

maksimr
Copy link
Contributor

@maksimr maksimr commented Apr 15, 2020

Fixes #7559

Signed-off-by: Maksim Ryzhikov rv.maksim@gmail.com

What it does

Print warning if user set incorrect theia target and explicitly set it to “browser”

How to test

Set incorrect target in package.json or pass it as an option. For example {“theia”: {“target”: “foo”}}

Review checklist

Reminder for reviewers

@akosyakov akosyakov added the theia-cli issues related to the theia-cli label Apr 15, 2020
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code looks very clean, I have not tried to run though, someone has to test

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.

I correctly see the following message when attempting to use foo as a target.
We however log the message twice:

@theia/example-browser: yarn run v1.22.4
@theia/example-browser: $ theia build --mode development
@theia/example-browser: Unknown target 'foo', target 'browser' to be used instead
@theia/example-browser: Unknown target 'foo', target 'browser' to be used instead
...

@maksimr
Copy link
Contributor Author

maksimr commented Apr 15, 2020

@vince-fugnitto I have relied on props cache so looks like ApplicationPackage instantiated twice. If there is a better place to put the code about warning it would be nice to know.

…icitly set it as browser

Fixes #7559

Signed-off-by: Maksim Ryzhikov <rv.maksim@gmail.com>
@akosyakov
Copy link
Member

I have relied on props cache so looks like ApplicationPackage instantiated twice. If there is a better place to put the code about warning it would be nice to know.

ApplicationPackage created in theia-cli and expose-loader via webpack in 2 different processes, so it is kind of expected

@akosyakov akosyakov merged commit 442e981 into eclipse-theia:master Apr 16, 2020
@maksimr maksimr deleted the issue-7559 branch April 16, 2020 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theia-cli issues related to the theia-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn about unknown target
3 participants