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

ProxyShallowContainer creates <undefined ></undefined> without a selector in the Component #105

Closed
duck-nukem opened this issue Apr 17, 2019 · 3 comments · Fixed by #106
Closed

Comments

@duck-nukem
Copy link
Collaborator

duck-nukem commented Apr 17, 2019

Example case:

// routes
export const ROUTES: Routes = [
  {
    path: ':id',
    component: MyPageComponent,
  },
]
// component
@Component({
  templateUrl: './my-page.component.html',
})
export class MyPageComponent {}

In this case the selector is not required, since the router will be the only one instantiating this component.

Since we have omitted the selector from the decorator, and we don't provide a template string when rendering with shallow-render, this code
<${directive.selector} ${inputBindings}></${directive.selector}> will return <undefined></undefined>.

What do you think would be best in this case? I tried falling back from directive.selector to 'proxy-shallow-container' for example, but angular rightfully stops me from doing that with a well placed 'shallow-proxy-host' is not a known element

duck-nukem added a commit to duck-nukem/shallow-render that referenced this issue Apr 17, 2019
duck-nukem added a commit to duck-nukem/shallow-render that referenced this issue Apr 17, 2019
@getsaf
Copy link
Owner

getsaf commented Apr 17, 2019

This is interesting. Not too long ago, we made every test component render in a host-component to solve for binding updates not firing lifecycle events. In this case, you're using an entry-component so maybe we don't need the host-component here but I'm not sure if that makes sense?

I see a couple of options here:

We could detect selector-less components and apply an overridden dynamic selector:

   const selector = resolvedTestComponent.selector || `${this._setup.testComponent.name}-selector`;
    if (!resolvedTestComponent.selector) {
      TestBed.overrideComponent(this._setup.testComponent, {
        set: { selector: `${this._setup.testComponent.name}-selector` }
      });
   }
   // Then we'd have to pass that selector into the _createTemplateString function
   private _createTemplateString(directive: Directive, selector: string, bindings: any) {/*...*/}

Alternatively, we could skip the whole host-component mechanism for entry-components since it's an unrealistic usage of rendering said component:
Change this to:

    const ComponentClass = resolvedTestComponent.selector
      ? createContainer(
          template || this._createTemplateString(resolvedTestComponent, finalOptions.bind),
          finalOptions.bind
        )
      : this._setup.testComponent;

I sort-of lean towards the second approach (for simplicity). I'm going to be pretty busy for the next couple of days and won't be able to get to this immediately. Feel free to give it a go and see if these solve it or if you have a better solution.

@getsaf
Copy link
Owner

getsaf commented Apr 17, 2019

Oh wow, you already have a PR. You rock!

@duck-nukem
Copy link
Collaborator Author

duck-nukem commented Apr 17, 2019

Thanks, I didn't yet consider the second option, if you're leaning towards that, I'm happy to amend the PR and implement that. Should be able to do that this week.

Actually, I think the conditional wrapping makes a lot more sense, I'll definitely go for that.

getsaf pushed a commit that referenced this issue Apr 18, 2019
* Components without selectors will have a default one assigned

Should fix #105

* Components can now only be wrapped in a container if they have a selector
Reverted a broader fix for the same issue
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 a pull request may close this issue.

2 participants