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

mini-browser: fix host pattern logic #9201

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Mar 15, 2021

When we set environment variables in our launch.json file, we use a
notation like "VAR": "${env:VAR}". VS Code will replace the template in
the value string with the content of the actual env var value, and if
not set it will expand to "". This caused issues with the logic that uses
this value since we expected to get undefined instead of an empty
string.

Closes #9052

How to test

Run the Launch Electron Backend debug configuration, it should work with this patch.

Without it should fail with "Failed to get cookie domain".

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added bug bugs found in the application electron issues related to the electron target mini-browser issues related to the mini-browser labels Mar 15, 2021
When we set environment variables in our launch.json file, we use a
notation like "VAR": "${env:VAR}". VS Code will replace the template in
the value string with the content of the actual env var value, and if
not set it will expand to "". This caused issues with the logic that
uses this value since we expected to get undefined instead of an empty
string.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
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 confirmed that #9052 is fixed with the following changes, I am now able to 'launch electron backend' from the launch configuration.

@paul-marechal paul-marechal merged commit 60fa885 into master Mar 15, 2021
@paul-marechal paul-marechal deleted the mp/debug-electron branch March 15, 2021 18:03
@github-actions github-actions bot added this to the 1.12.0 milestone Mar 15, 2021
@kittaakos
Copy link
Contributor

Thanks for the fix.

What about this location? Is this looking good?

const pattern = process.env[MiniBrowserEndpointNS.HOST_PATTERN_ENV] ?? MiniBrowserEndpointNS.HOST_PATTERN_DEFAULT;

@paul-marechal
Copy link
Member Author

@kittaakos we should change it too indeed. After looking into it, what doesn't help is that we set env variables in our launch.json that expand to empty string when vars are not set, which doesn't really sound useful to me. In most cases the env var should either be set or unset. I'll make another PR to address both points.

@kittaakos
Copy link
Contributor

I'll make another PR to address both points.

Thank you for looking into it and for the future fix.


Some context:
It did not cause any issues, I am a just few commits behind the master HEAD, and have to manually apply your fix in the source: I accidentally found this occurrence. So no hurry with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application electron issues related to the electron target mini-browser issues related to the mini-browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot start the electron app from the launch config
3 participants