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

Fix case where autoRun is false but no other addon set content into the {{content-for 'app-boot'}} #710

Merged
merged 2 commits into from
Mar 4, 2021

Conversation

thoov
Copy link
Collaborator

@thoov thoov commented Mar 4, 2021

When this.appBoot is an empty string this would result in a falsy condition when instead it should be considered truthy. This code path is uncovered when autoRun is set to false due to:

appBoot: !this.adapter.autoRun() ? this.adapter.appBoot() : '',

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 4, 2021

I know we poked at this together but I think we'll need a bit more details here before landing.

string which was being checked for as falsy when it should actually
be considered truthy.
@thoov thoov marked this pull request as ready for review March 4, 2021 22:25
@thoov thoov added the bug Something isn't working label Mar 4, 2021
}

readAppBoot() {
if (!this.appBoot) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, the prior guard had issues because in cases where no addon emits {{content-for 'app-boot'}} content the value will be '', which is going to evaluate to falsey.

@rwjblue rwjblue merged commit 1806752 into embroider-build:master Mar 4, 2021
@rwjblue rwjblue changed the title Fix case where autoRun = false Fix case where autoRun is false but no other addon set content into the {{content-for 'app-boot'}} Mar 4, 2021
@thoov thoov deleted the autorun-false branch March 4, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants