-
-
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
[BUGFIX beta] Make link-to a real component. #12176
Conversation
// now that we have the component instance. | ||
layout = get(component, 'layout') || layout; | ||
component._hasBlock = !!templates.default; |
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.
This should be provided as an option in createOptions
(so that it is available at init
and doesn't change the shape of the object after initialization).
We chatted a little bit about this in slack this evening, I think we have a path forward to allowing simple extending of |
|
||
// TODO: Remove once `hasBlock` is working again | ||
attrs.hasBlock = !!template; | ||
Ember.runInDebug(() => { |
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.
Is this kosher? This is only used for an error message inside an Ember.Deprecate
call.
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.
Ya, seems fine. Can you also underscore (so hash._view)?
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.
Is our build story clever enough to drop an import if it's only used inside of a runInDebug
? We could move this to a symbol and stop squatting on properties.
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.
I believe the import would still exist, but I think that is likely fine. Using a symbol here seems good (and helps prevent folks that we are going to start encouraging to use Ember.LinkComponent.extend
from relying on the private internals...).
794682f
to
04642c1
Compare
@@ -0,0 +1,5 @@ | |||
import { symbol } from 'ember-metal/utils'; |
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.
This doesn't seem like a compat type thing to me, maybe we can move it to packages/ember-views/lib/system/
instead?
04642c1
to
46829d4
Compare
I went with [BUGFIX beta] as this changes nothing and closes #11962. |
import { symbol } from 'ember-metal/utils'; | ||
|
||
// These symbols will be used to limit link-to's public API surface area. | ||
let HAS_BLOCK = symbol('HAS_BLOCK'); |
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.
These can be export let HAS_BLOCK...
(instead of separate declaration and export statements).
👍 |
46829d4
to
a805e5f
Compare
Can you also add |
a805e5f
to
c8bdb32
Compare
[BUGFIX beta] Make link-to a real component.
Good work. Hopefully with @ef4's strategy we can drop the HAS_BLOCK symbol entirely. |
Once we have "splat" (/at this point I know it pretty well and might just pick up where he left off.) |
I think you can just copy the attrs onto the |
Alright, I commit to bringing this home. To-do list from #11962 plus one more: