Passing optional component arguments through does not work

[mydea]

I am currently trying to write a PR for ember-power-select to work fully with embroider. However, I am running into this somewhat unexpected issue when trying to use the addon with embroider & staticComponents=true.

A component accepts an optional sub-component. I can realize this with @embroider/util like this:

{{! addon/components/power-select.hbs }}

{{#if @afterOptionsComponent}}
  {{#let (component (ensure-safe-component @afterOptionsComponent)) as |AfterOptions|}}
    <AfterOptions
      @extra={{@extra}}
      @select={{publicAPI}}
    />
  {{/let}}
{{/if}}

Now if I have another component which should allow to (optionally) pass this through, like this:

{{! addon/components/power-select-multiple.hbs }}
<PowerSelect
  @afterOptionsComponent={{@afterOptionsComponent}}
/>

It complains when running ember b:

Build Error (PackagerRunner) in ../../components/power-select-multiple.hbs

Module Error (from ember-power-select/node_modules/thread-loader/dist/cjs.js):
argument "afterOptionsComponent" to component "PowerSelect" is treated as a component, 
but the value you're passing is dynamic: @afterOptionsComponent in 
$TMPDIR/embroider/e23032/components/power-select-multiple.hbs/power-select-multiple.hbs

This happens with any argument really, also for other similar arguments. Interestingly, it even seems to happen when I remove any usage of @afterOptionsComponent in the addon (meaning it is not used anywhere except in @afterOptionsComponent={{@afterOptionsComponent}} in power-select-multiple). If I rename every occurrance to e.g. @afterOptionsThing, it also works again (but complains about @beforeOptionsComponent). So not quite sure what is going on there..!

You can see a reproduction in my branch: https://github.com/mydea/ember-power-select/tree/fn/ensure-safe-components just check it out, run npm install and ember b and you'll see a bunch of errors.

[ef4]

Thanks for working on this. I think you're getting interference from the packageRules for ember-power-select that work around the problems you are fixing. You can disable the rules in ember-cli-build.js like:

return require('@embroider/compat').compatBuild(app, Webpack, {
    staticAddonTestSupportTrees: true,
    staticAddonTrees: true,
    staticHelpers: true,
    staticComponents: true,
+   packageRules: [{
+     package: 'ember-power-select',
+     semverRange: '*',
+   }, {
+     package: 'ember-basic-dropdown',
+     semverRange: '*'
+   }]
  });

This is registering empty rules for those packages, which take precedence over the built-in rules that ship with embroider. Once your changes are in an ember-power-select release, we can upstream this into embroider with the right semverRange so it knows that the new-enough ember-power-select doesn't need packageRules.

With that change, the dynamic component errors go away on your branch, but then I hit #849, which could be worked around for now by downgrading ember-source to 3.26.

[mydea]

Hmm, I don't think that is enough, as it still does not allow anything it deems "dynamic" to be passed to the arguments. I wrote up a PR: #922 which as far as I can see should fix the issue and should make sense & hopefully not break the static analyzability of component usage 😅

Basically the PR allows the following previously disallowed usages for staticComponents: true:

<PowerSelect
  @afterOptionsComponent={{if 
    @afterOptionsComponent 
    (ensure-safe-component @afterOptionsComponent) 
    undefined
  }}
  @afterOptionsComponent={{null}}
  @afterOptionsComponent={{undefined}}
  @afterOptionsComponent={{unless @thing (ensure-safe-component @afterOptionsComponent)}}
  @afterOptionsComponent={{ensure-safe-component @afterOptionsComponent}}
/>

Let me know if I am missing something or if that doesn't actually make as much sense as I thought it would!

With that PR, the following things work:

{{! pass optional thing through }}
<PowerSelect 
  @afterOptionsComponent={{ensure-safe-component @afterOptionsComponent}} 
/>

{{! use non-undefined default }}
<PowerSelect 
  @beforeOptionsComponent={{if @beforeOptionsComponent (ensure-safe-component @beforeOptionsComponent) null}} 
/>

[mydea]

Or maybe I misunderstand something there - it does seem to build for me too when I remove the ember-basic-dropdown/ember-power-select custom rules, but should it? I added test cases covering this in my PR which failed for generic rules there 🤔

[ef4]

Two things are combining to cause confusion:

• I think the packageRules that ship in embroider are slightly out of date with upstream changes in ember-power-select, so they will complain with or without your changes.
• packageRules are only needed when you're working around addons that don't natively ensure dynamic component safety. I think ember-basic-dropdown has already updated to use ensure-safe-component, and you're updating ember-power-select, so the rules aren't needed anymore.

So yes, I think it's fine that it works with the rules disabled. It would also be OK to update the rules so they pass again, but since ensure-safe-component is already protecting users, it's better to drop the rules.

[void-mAlex]

hello @mydea doing a bit of housekeeping here and wondering if this can be closed now as it seems to have had several PRs merged to address various issues

[mydea]

I think we can close this!

[Mikek2252]

With ember-power-select being v2 addon should the package rules still be in embroider. or at least should there be some version check to only apply them for versions that aren't v2?