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

Local template blocks (as a precursor to block slot syntax) #199

Closed
wants to merge 5 commits into from

Conversation

machty
Copy link
Contributor

@machty machty commented Jan 14, 2017

@machty machty changed the title Local template blocks Local template blocks (as a precursor to block slot syntax) Jan 14, 2017
@knownasilya
Copy link
Contributor

I do agree that this is the right way to start providing more composability options, and I'm fine if we
do the slot sugar stuff later, because the functionality is more important I think.


`let-block` declarations behave similar to block params in that a reference to `fooBlock` in the above template will always reference the block declared by `let-block` and never try and do a property lookup of `fooBlock` on the context.

`let-block` declarations are hoisted to the top of the current lexical scope, like classic JS function declarations; this means it's possible to reference a block _above_ where it is defined.
Copy link
Member

@GavinJoyce GavinJoyce Jan 14, 2017

Choose a reason for hiding this comment

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

What's a use case for hoisting let-blocks?

this means it's possible to reference a block above where it is defined

This may be confusing as it seems to be inconsistent with JavaScript let hoisting and temporal dead zones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I go back and forth on this. If we don't hoist then we have to do one of the following:

  1. References to fooBlock before let-block fooBlock will refer to some outer scope, which seems bad and surprising
  2. We detect that there is a let-block fooBlock and throw a compile error if fooBlock is referenced above the let-block. Maybe this is fine?

Copy link
Contributor

@knownasilya knownasilya Jan 14, 2017

Choose a reason for hiding this comment

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

Maybe just rename the helper? Like {{#define-block

Copy link
Member

@GavinJoyce GavinJoyce Jan 14, 2017

Choose a reason for hiding this comment

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

If we can enforce let like semantics in the compiler this would simplify the conceptual model quite a bit I think

Copy link
Member

Choose a reason for hiding this comment

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

I much prefer the second option.

Copy link
Member

Choose a reason for hiding this comment

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

In ember-let, inline-let's declaration last until the nearest block/element terminates. This aligns nicely with how people commonly indent their Handlebars source.

Here's some examples: thefrontside/ember-let#12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmun Doesn't the inline form of ember-let do Option 1, e.g. before the let, references to a or b will "pierce" the lexical scope?

{{/let-block}}

{{! render fooBlock here...}}
{{yield 'A' 'B' cValue to=fooBlock}}
Copy link

Choose a reason for hiding this comment

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

I'm curious if you'd be able to render a block simply by placing it in the eval position of an expression:

{{fooBlock 'A' 'B' cValue}}

Copy link
Contributor

Choose a reason for hiding this comment

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

or even {{my-comp customBlock=(fooBlock 'a' 'b')}} where the component provides only 'c'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cowboyd That syntax may or may not involve to huge of changes to Glimmer... i.e. it'd be hard to add that feature without possibly opening the Pandora's box of a generic Renderables/Invocables interface (which might sound nice but might prematurely bloat the scope of this RFC.

@knownasilya Hmm, you know, you've given me an idea... gonna follow up in top-level comment and mention you there.

Add another demand/need lol
@webark
Copy link

webark commented Jan 15, 2017

let-block does seem a bit confusing. It took me a minute to realize it was meaning to define a block and assign it to a local variable. The pseudo defining of a variable/block does also seem a little..... :/ I can see the power of it, but i'm personally a little weary of any abstraction in the view layer, cause you are defining markup outside of any context to where it will end up (as apposed to the slot syntax). just don't want to muddy the view layer too much.

Personal opinions aside, It could be helpful to throw a compile error/warn when a block is defined but not ever called? Since they are decoupled a bit, it could aide with keeping the templates clean and reduce potential waste. i could see people refactoring/removing a component call and forgetting/not knowing that a block had been defined somewhere else in the template that is no longer being used.

@machty
Copy link
Contributor Author

machty commented Jan 15, 2017

@webark I like the idea about error/warn, preferably only a warning since I can imagine jotting out the various blocks I'd need before deciding who/what to pass it to, and it'd be annoying if the compiler refused to compile until it was all wired up.

I agree that we should tread lightly with any new abstractions brought to Glimmer, but keep in mind that any nested slot syntax is going to miss the use case of declaring a block and passing it to multiple components, which, in my experience as an app and addon, is a deal-breaker.

And while we are definitely defining markup outside all of the components we may pass it too, this RFC preserves data context (e.g. all the values accessible to these blocks) in a consistent, safe, and familiar way (i.e. any additional data access takes place in the form of block params).

@machty
Copy link
Contributor Author

machty commented Jan 15, 2017

After having some time to mull it over, one major detail that irks me is that we are building out two incongruent APIs for passing "renderables" into higher-order components:

  1. Passing a fooComp=(component 'x-foo') into another component, which renders it using {{component fooComp additional=values}}
  2. Passing in fooBlock=someLocalLetBlockVar into another component, which renders it using
    {{yield positional param to=fooBlock}}

Why should HOCs (higher order components; components that render other components passed into them) have to distinguish between these two types of renderables? And why should you have to use {{component}} in one case and {{yield}} in the other?

Furthermore, let's imagine we unified the API for rendering component/blocks passed in as attrs (either by making {{component}} helper smart enough to render let-block blocks, or making {{yield}} helper smart enough to render (component)s), we'd still have a divide where blocks expect to be passed a list of args in the form of block params and components expect to be passed as hash of args by way of merging the args into the component's properties (and possibly using the esoteric positionalParams API).

I'm worried that this RFC in its present state will continue to build out functionalilty on the yield-renderable side of things, widening the chasm between yield-renderables and component-renderables, which seems... obviously bad. And the same thing would likely happen if we added a slot syntax (which are dressed up yield-renderables).

I have some ideas for how we might resolve this, but I wanted this to stand alone as its own comment for easier absorption.

@kybishop
Copy link

@runspired this seems like a direct replacement for flexi-sustain

@cowboyd
Copy link

cowboyd commented Jan 16, 2017

Why should HOCs (higher order components; components that render other components passed into them) have to distinguish between these two types of renderables? And why should you have to use {{component}} in one case and {{yield}} in the other?

Would this adressed if the way to render was just to place it in the eval position of an expression?

{{component fooComp additional=values}} -> {{fooComp additional=values}}
{{yield positional param to=fooBlock}} -> {{fooBlock positional param}}

I'm probably ignorant of some reason why this couldn't work, but it certainly seems simple enough.

Edit: nvm, I did not see your response to my previous comment.

@machty
Copy link
Contributor Author

machty commented Jan 16, 2017

@cowboyd That might work but it would just push the problem to a different layer; whereas without your proposed syntax an HOC would have to decide/know/remember whether to render via {{component}} or {{yield}}, it would now still have to make this distinction by having to decide whether to render with positional params or a named param hash.

One possible alternative is to stop building out the yield-renderable side of things and standardize on patterns that are component-centric (and proven/familiar). Instead of let-block defining local blocks, we add let-component which defines a local component that extends the default Ember.Component class with a layout template of {{yield this}}. For example:

{{#let-component fooComp bar=123 as |c|}}
  bar={{c.bar}}
{{/let-component}}

You could render this component right away as
if it were any other component via:

{{component fooComp}}
// renders <div>bar=123</div>

And as always you can override default attrs with:

{{component fooComp bar=456}}
// renders <div>bar=456</div>

If you didn't want the div-wrapping you could do

{{component fooComp tagName=""}}
// renders "bar=456"

(or possibly we make tagName="" the default; maybe this plays well with future glimmer component semantics, I don't know.)

So the reason this is nice is that all the present-day HOCs that already expect (component) attrs don't have to change a single thing about how they work / how they "pass" attrs; you can take something that used to have to live in a separate file and refactor as such:

Before:
{{complex-component
   header=(component 'tiny-header')
   footer=(component 'tiny-footer')}}

After:
{{#let-component headerComp as |c|}}
  <h1>{{c.title}}</h1>
{{/let-component}}
{{#let-component footerComp as |c|}}
  <h1>{{c.footerThing}}</h1>
{{/let-component}}

{{complex-component header=headerComp footer=footerComp}}

(If it's not clear why this is a desired refactor, keep in mind that the present day API forces you to split out components with minuscule amounts of logic/DOM just so you can pass in overridable bits of DOM via the (component) API.

@machty
Copy link
Contributor Author

machty commented Jan 16, 2017

I know the whole point of this RFC is to side-step slot syntax, but I figured I'd follow up with what I see as a potentially nice pattern/convention for building components with overridable internals.

https://gist.github.com/machty/c00c83f3fbefefa72cefb0bb2322fc4f

@machty
Copy link
Contributor Author

machty commented Jan 18, 2017

Due to the uncannily high quality level of feedback and collaboration taking place in the #topic-forms channel in the Ember Community Slack, it has become evident that there other primitives / syntactical enhancements that should be made to Ember/Glimmer that would render the ideas in this RFC moot. Stay tuned for such enhacements in another RFC.

@machty machty closed this Jan 18, 2017
@machty
Copy link
Contributor Author

machty commented Jan 20, 2017

Here is the followup RFC. Wish me luck.

@mmun
Copy link
Member

mmun commented Jan 20, 2017

Good luck.

@machty
Copy link
Contributor Author

machty commented Jul 30, 2017

FYI this RFC eventually evolved into the now-merged Named Blocks RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants