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

Errors with embroider and ember-table / ensure-safe-component / TypeScript #1336

Closed
RobbieTheWagner opened this issue Jan 25, 2023 · 15 comments

Comments

@RobbieTheWagner
Copy link

I am hitting some very specific error cases where when I try to pass a cellComponent to ember-table, which is wrapped in ensure-safe-component, I get Uncaught Error: Attempted to load a component, but there wasn't a component manager associated with the definition. The definition was: factory.

If I step through the error, it hits a couple lines inside ember-table where it is registering a container.

This error only occurs if the component I am trying to use as a cellComponent has a TypeScript backing class file. If I remove the TS file and use JS instead or have no backing class, the error goes away.

I made a reproduction repo here for debugging:

https://github.com/rwwagner90/embroider-bug

@NullVoxPopuli
Copy link
Collaborator

Something I noticed while debugging this:
I turned all the static flags off, got this error:


Error occurred:

- While rendering:
  -top-level
    application
      table-wrapper
        ember-table
          ember-tbody
            vertical-collection
              ember-table-private/row-wrapper
                ember-tr
                  ember-td

Uncaught Error: Attempted to load a component, but there wasn't a component manager associated with the definition. The definition was: factory

With the static flags on, the error is the same, except the template stack is terrible:



Error occurred:

- While rendering:
  -top-level
    application
      TableWrapperComponent
        Class
          Class
            Class
              Class
                Class
                  Class

@NullVoxPopuli
Copy link
Collaborator

I removed ensure-safe-component (with static flags all off, so testing more compatibility than strictness):

and the error persists:

Uncaught Error: Attempted to load a component, but there wasn't a component manager associated with the definition. The definition was: factory
      <r.cell class='h-10' as |cell column row cellMeta|>
        {{#if column.cellComponent}}
          {{log column.cellComponent}}
          {{#let (component column.cellComponent) as |Wrapper|}}
            {{log Wrapper}}
            <Wrapper>
              {{cell}}
            </Wrapper>
          {{/let}}
        {{else}}
          {{cell}}
        {{/if}}
      </r.cell>

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Jan 25, 2023

The compiled file with backing TS class
import { setComponentTemplate } from '@ember/component';
import { precompileTemplate } from '@ember/template-compilation';
import Component from '@glimmer/component';

var TEMPLATE = precompileTemplate("<EmberTable as |t|>\n  <t.head\n    @scrollIndicators={{@scrollIndicators}}\n    @widthConstraint={{@widthConstraint}}\n    @fillMode={{@fillMode}}\n    @columns={{@columns}}\n    @sorts={{this.sorts}}\n    @onUpdateSorts={{fn (mut this.sorts)}} as |h|\n  >\n    <h.row class=\'text-gray-700 text-xs dark:text-gray-200\' as |r|>\n      <r.cell class=\'py-2 text-left\' as |column columnMeta|>\n        <div class=\'flex items-center\'>\n        </div>\n      </r.cell>\n    </h.row>\n  </t.head>\n\n  <t.body @rows={{@rows}} as |b|>\n    <b.row\n      class=\'text-gray-700 text-xs even:bg-gray-100 odd:bg-gray-200 dark:even:bg-gray-600 dark:odd:bg-gray-700 dark:text-gray-200\' as |r|\n    >\n      <r.cell class=\'h-10\' as |cell column row cellMeta|>\n        {{#if column.cellComponent}}\n          {{log column.cellComponent}}\n          {{#let (component\n            column.cellComponent\n          ) as |Wrapper|}}\n            {{log Wrapper}}\n            <Wrapper>\n              {{cell}}\n            </Wrapper>\n          {{/let}}\n        {{else}}\n          {{cell}}\n        {{/if}}\n      </r.cell>\n    </b.row>\n  </t.body>\n</EmberTable>");

class TableWrapperComponent extends Component {}
setComponentTemplate(TEMPLATE, TableWrapperComponent);

export { TableWrapperComponent as default };
//# sourceMappingURL=index.js.map

When trying to delete the TS file, I got a build error from rollup:

[!] (plugin Typescript) RollupError: No inputs were found in config file '/home/nullvoxpopuli/Development/tmp/embroider-bug/embroider-bug/tsconfig.json'. Specified 'include' paths were '["src/**/*","types/**/*"]' and 'exclude' paths were '["./dist"]'.

I checked the publicEntrypoints, and saw that it was erroneously specifying ts extensions instead of JS.
I added the TS filfe back, changed the rollup config,
But the output didn't change:

The output didn't change
import { setComponentTemplate } from '@ember/component';
import { precompileTemplate } from '@ember/template-compilation';
import Component from '@glimmer/component';

var TEMPLATE = precompileTemplate("<EmberTable as |t|>\n  <t.head\n    @scrollIndicators={{@scrollIndicators}}\n    @widthConstraint={{@widthConstraint}}\n    @fillMode={{@fillMode}}\n    @columns={{@columns}}\n    @sorts={{this.sorts}}\n    @onUpdateSorts={{fn (mut this.sorts)}} as |h|\n  >\n    <h.row class=\'text-gray-700 text-xs dark:text-gray-200\' as |r|>\n      <r.cell class=\'py-2 text-left\' as |column columnMeta|>\n        <div class=\'flex items-center\'>\n        </div>\n      </r.cell>\n    </h.row>\n  </t.head>\n\n  <t.body @rows={{@rows}} as |b|>\n    <b.row\n      class=\'text-gray-700 text-xs even:bg-gray-100 odd:bg-gray-200 dark:even:bg-gray-600 dark:odd:bg-gray-700 dark:text-gray-200\' as |r|\n    >\n      <r.cell class=\'h-10\' as |cell column row cellMeta|>\n        {{#if column.cellComponent}}\n          {{log column.cellComponent}}\n          {{#let (component column.cellComponent) as |Wrapper|}}\n            {{log Wrapper}}\n            <Wrapper>\n              {{cell}}\n            </Wrapper>\n          {{/let}}\n        {{else}}\n          {{cell}}\n        {{/if}}\n      </r.cell>\n    </b.row>\n  </t.body>\n</EmberTable>");

class TableWrapperComponent extends Component {}
setComponentTemplate(TEMPLATE, TableWrapperComponent);

export { TableWrapperComponent as default };
//# sourceMappingURL=index.js.map

So now, I delete the backing TS, but I still need "a" ts file, so I added an index.ts at the root of the addon.

The compiled file without the TS backing class (template-only)
import { setComponentTemplate } from '@ember/component';
import { precompileTemplate } from '@ember/template-compilation';
import templateOnly from '@ember/component/template-only';

var TEMPLATE = precompileTemplate("<EmberTable as |t|>\n  <t.head\n    @scrollIndicators={{@scrollIndicators}}\n    @widthConstraint={{@widthConstraint}}\n    @fillMode={{@fillMode}}\n    @columns={{@columns}}\n    @sorts={{this.sorts}}\n    @onUpdateSorts={{fn (mut this.sorts)}} as |h|\n  >\n    <h.row class=\'text-gray-700 text-xs dark:text-gray-200\' as |r|>\n      <r.cell class=\'py-2 text-left\' as |column columnMeta|>\n        <div class=\'flex items-center\'>\n        </div>\n      </r.cell>\n    </h.row>\n  </t.head>\n\n  <t.body @rows={{@rows}} as |b|>\n    <b.row\n      class=\'text-gray-700 text-xs even:bg-gray-100 odd:bg-gray-200 dark:even:bg-gray-600 dark:odd:bg-gray-700 dark:text-gray-200\' as |r|\n    >\n      <r.cell class=\'h-10\' as |cell column row cellMeta|>\n        {{#if column.cellComponent}}\n          {{log column.cellComponent}}\n          {{#let (component column.cellComponent) as |Wrapper|}}\n            {{log Wrapper}}\n            <Wrapper>\n              {{cell}}\n            </Wrapper>\n          {{/let}}\n        {{else}}\n          {{cell}}\n        {{/if}}\n      </r.cell>\n    </b.row>\n  </t.body>\n</EmberTable>");

var index = setComponentTemplate(TEMPLATE, templateOnly());

export { index as default };
//# sourceMappingURL=index.js.map

And now the app errors with:



Error occurred:

- While rendering:
  -top-level
    application
      table-wrapper
        ember-table

Uncaught Error: Assertion Failed: You can only pass a path to mut

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Jan 25, 2023

But deleting the

    @onUpdateSorts={{fn (mut this.sorts)}}

brings us back to:



Error occurred:

- While rendering:
  -top-level
    application
      table-wrapper
        ember-table
          ember-tbody
            vertical-collection
              ember-table-private/row-wrapper
                ember-tr
                  ember-td
Uncaught Error: Attempted to load a component, but there wasn't a component manager associated with the definition. The definition was: factory

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Jan 25, 2023

As a test to ensure that your component is valid, I deleted the application.hbs in the app, and just rendered

<TableCells::CheckboxCell />

it is valid. 🎉

To test strict-mode, I changed that to:

<this.TableCheckboxCell />

and got this error:

Uncaught Error: Expected a dynamic component definition, but received an object or function that did not have a component manager associated with it. The dynamic invocation was `<this.TableCheckboxCell>` or `{{this.TableCheckboxCell}}`, and the incorrect definition is the value at the path `this.TableCheckboxCell`, which was: factory

which matches what ember-table is doing

@NullVoxPopuli
Copy link
Collaborator

To debug what's different between local reference, and container lookup:

{{log this.TableCheckboxCell}}

{{log (component 'table-cells/checkbox-cell')}}

image

@NullVoxPopuli
Copy link
Collaborator

so, breaking the error down a bit, I suppose this could happen if there were multiple ember-source entries, providing multiple @ember/component -- which would indeed mean that the component manager is not wired up to your app's list of component managers

@NullVoxPopuli
Copy link
Collaborator

I noticed the addon does not declare a peer on ember-source

@NullVoxPopuli
Copy link
Collaborator

I then had to add

"dependenciesMeta": {
  "embroider-bug": {
    "injected": true
  }
}

To the app so that peers may resolve correctly.

Starting the app, I now get a build time error:

Build Error (PackagerRunner) in components/ember-table-loading-more.js

Module not found: Error: Can't resolve '../../node_modules/.pnpm/ember-table@5.0.1/node_modules/ember-table/components/ember-table-loading-more/component' in '$TMPDIR/embroider/a2a88b/test-app/components/ember-table-loading-more.js'

@NullVoxPopuli
Copy link
Collaborator

That file referenced is here:

❯ ls ./node_modules/.pnpm/ember-table@5.0.1/node_modules/ember-table/
addon               app           config    jsconfig.json  package.json  tsconfig.json
addon-test-support  CHANGELOG.md  index.js  LICENSE.md     README.md     types

This is the addon root, not the package root (which is a merge of addon + addon-test-support).

I'd wager that due to ember-table being in the pods format resolution has failed.

I have a big hunch that moving the addon to colocated components will fix the problem.

@ef4
Copy link
Contributor

ef4 commented Jan 26, 2023

Firstly, there's no reason to ever use ensure-safe-component in an app on ember-source >= 3.25. It continues to exist only for addons that need to support older ember versions. For example:

diff --git a/embroider-bug/src/components/table-wrapper/index.hbs b/embroider-bug/src/components/table-wrapper/index.hbs
index 7d1f840..3ca49ad 100644
--- a/embroider-bug/src/components/table-wrapper/index.hbs
+++ b/embroider-bug/src/components/table-wrapper/index.hbs
@@ -11,9 +11,9 @@
       <r.cell class='py-2 text-left' as |column columnMeta|>
         <div class='flex items-center'>
           {{#if column.headerComponent}}
-            {{#component (ensure-safe-component column.headerComponent)}}
+            <column.headerComponent>
               {{column.heading}}
-            {{/component}}
+            </column.headerComponent>
           {{else}}
             {{column.heading}}
           {{/if}}
@@ -44,14 +44,9 @@
     >
       <r.cell class='h-10' as |cell column row cellMeta|>
         {{#if column.cellComponent}}
-          {{#component
-            (ensure-safe-component column.cellComponent)
-            cellMeta=cellMeta
-            column=column
-            row=row
-          }}
+          <column.cellComponent @cellMeta={{cellMeta}} column={{column}} row={{row}}>
             {{cell}}
-          {{/component}}
+          </column.cellComponent>
         {{else}}
           {{cell}}
         {{/if}}

But second, ensure-safe-component is a red herring here and has nothing to do with the bug. If we step through it's code, we see that it's making itself a no-op as intended.

Third, this:

I'd wager that due to ember-table being in the pods format resolution has failed.

Also has nothing to do with the bug.

The CheckboxCell component isn't getting built correctly. If you look at its source in the build output it's only the .hbs part, without its .ts part.

We can confirm this is the problem by manually connected the two parts together and seeing that it works:

diff --git a/test-app/app/components/table-cells/checkbox-cell/index.hbs b/test-app/app/components/table-cells/checkbox-cell/index.hbs
deleted file mode 100644
index 81f7ebf..0000000
--- a/test-app/app/components/table-cells/checkbox-cell/index.hbs
+++ /dev/null
@@ -1,3 +0,0 @@
-<div class='ml-2 sm:ml-4 md:ml-6'>
-  <input type='checkbox' />
-</div>
\ No newline at end of file
diff --git a/test-app/app/components/table-cells/checkbox-cell/index.ts b/test-app/app/components/table-cells/checkbox-cell/index.ts
index 9976c6c..c631c30 100644
--- a/test-app/app/components/table-cells/checkbox-cell/index.ts
+++ b/test-app/app/components/table-cells/checkbox-cell/index.ts
@@ -8,6 +8,14 @@ interface CheckboxCellSignature {
 
 const CheckboxCell = templateOnlyComponent<CheckboxCellSignature>();
 
+import { hbs } from 'ember-cli-htmlbars';
+import { setComponentTemplate } from "@ember/component";
+const template = hbs(`<div class='ml-2 sm:ml-4 md:ml-6'>
+<input type='checkbox' />
+</div>`)
+
+setComponentTemplate(CheckboxCell, template );
+
 export default CheckboxCell;
 
 declare module '@glint/environment-ember-loose/registry' {

@ef4 ef4 closed this as completed Jan 26, 2023
@ef4 ef4 reopened this Jan 26, 2023
@ef4
Copy link
Contributor

ef4 commented Jan 26, 2023

(I hit close by accident, this is an open embroider bug.)

@RobbieTheWagner
Copy link
Author

@ef4 so this is indeed a bug with embroider and not something I need to fix in our code or in ember-table?

@ef4
Copy link
Contributor

ef4 commented Jan 26, 2023

Correct.

ef4 added a commit that referenced this issue Jan 27, 2023
I don't really know what 7882a2d was trying to achieve and it definitely looks weird and it's the cause of #1336 and probably #1341.

Maybe CI will tell me why this was done in the first place...
@ef4
Copy link
Contributor

ef4 commented Jan 27, 2023

Fixed in #1342 and confirmed against this issue reproduction repo.

@ef4 ef4 closed this as completed Jan 27, 2023
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

No branches or pull requests

3 participants