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

ensureSafeComponent helper #571

Merged
merged 17 commits into from
Oct 21, 2020
Merged

ensureSafeComponent helper #571

merged 17 commits into from
Oct 21, 2020

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Oct 16, 2020

This introduces a helper function we can tell addon authors to use to make their dynamic component invocations more future-proof.

Copy link
Collaborator

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

This is great!

I noticed that something like that was planned in #487, but didn't expect this to be available that soon. otherwise I would have waited for this before doing ember-bootstrap/ember-bootstrap#1251, where I basically pulled all component references from JS to templates (in a statically analyzable way) to enable static components mode...

compute([value]) {
return ensureSafeComponent(value, this);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this implementation live in the addon folder and be reexported from the app folder, according to common addon conventions, to ensure this is transpiled by Babel?

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 agree that would be more conventional, and I should probably make the change you suggest. This started out as a functional helper, in which case there really was basically no implementation to worry about. But then it became a class-based helper, and now has a bunch of syntax that might in theory be problematic for an app.

But I don't think this actually breaks under any real conditions. The thing about the app folder isn't that it doesn't get transpiled, it's that it gets transpiled under the app's babel config and not the addon's. But what is the minimum level of Javascript features we can assume will work in the app? We already know that the app must support ES modules because people put reexport syntax in the app tree. Are there really any apps that support ES modules but not class syntax or array destructuring? Probably not.


## Replacing the {{component}} helper

Using the `{{component}}` helper (with one small exception which we'll get to) prevents an addon from achieving the **Optimized Embroider Safe** support level. So addons should stop using it. But how?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is not quite true, i.e. there are at least two valid exceptions: the one you mention (static string), but also when yielding contextual components you will still need the {{component}} helper to curry arguments. And this will still work with static components, as long as the component positional argument itself is guaranteed to refer to a component that embroider was able to "see" during its analysis.

Example: https://github.com/kaliber5/ember-bootstrap/pull/1251/files#diff-c3c2155cba3ac783f3443fa7014f24ced9dac371164096f02cb9d1c0767a651eR8-R9.

Admittedly there is a loophole here as @titleComponent could be a string, but I guess I could now wrap that with the ensureSafeComponent helper, right? Anyways, as the example shows you still need the component helper to curry arguments. FWIW, I have that pattern quite a lot in ember-bootstrap, where contextual components are yielded, but they can also be passed as arguments to exchange the used component class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. We should expand this section of the guide to help people with those cases. While there are plenty of patterns that are safe, there really is only one that embroider can see is safe, and that is a string literal argument.

We need to think about what the future of currying looks like in Ember, because I suspect it doesn't need to continue being a special feature of the {{component}} helper.

Once we're free to pass around component classes and then invoke them, "currying" can just be class extends.

Also, I think named blocks replaces many cases where you would have passed a curried component as an argument:

<Menu @titleBar={{component "fancy-bar" mode=this.mode}} />
{{!-- vs --}}
<Menu>
  <:titleBar>
    <FancyBar @mode={{this.mode}} />
  </:titleBar>
</Menu>

Notice that this also solves the problem of "how do I curry HTML attributes?".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment makes me realize I do need to explain the rule more precisely.

You can use the component helper, but only if the first argument is:

  • a string literal
  • or a call to the ensure-safe-component helper.

I didn't say so explicitly, but embroider should be made to not worry about this pattern:

{{component (ensure-safe-component @whatever) }}

I didn't mention that case because I was assuming it would never be needed (use angle brackets instead), but you're right that if you want to curry arguments onto it but not invoke it, you'd need that pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now implemented and documented

@ef4 ef4 merged commit ff02a8d into master Oct 21, 2020
@ef4 ef4 deleted the addon-utils branch October 21, 2020 20:39
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.

2 participants