-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[FEATURE set-component-template] #18158
Conversation
dad92ae
to
8423694
Compare
8423694
to
4b4ab3e
Compare
Co-authored-by: Robert Jackson <[email protected]>
We explicitly added `| undefined` to the return type in the previous commit Co-authored-by: Robert Jackson <[email protected]>
4b4ab3e
to
6c3ac7c
Compare
Fixed merge conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good! Just one minor question, everything else is 👍
|
||
function lookupComponent(owner: Owner, name: string, options: LookupOptions): Option<LookupResult> { | ||
if (options.source || options.namespace) { | ||
if (EMBER_MODULE_UNIFICATION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these? I thought we'd removed them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pzuraq - We disabled the flag (though when we did that @chancancode and I had already done the work here to support it) because it was impossible to test either colocation or MU (we only test with all optionals or none) and some aspects of the feature conflicts with MU.
tldr; we haven't removed the MU feature flagged code yet, and just in case we need to salvage parts of it (who knows why) we wanted to leave things roughly in the same basic state before/after this PR
Implements the Ember side of RFC #481.